Skip to content

Commit 620228b

Browse files
fix(apple): key the toolchain fingerprint memo on the resolved xcode-select dir (#3726)
* fix(apple): key the toolchain fingerprint memo on the resolved xcode-select dir `toolchainFingerprintCacheKey` keyed the memo on `DEVELOPER_DIR` alone, so every default `xcode-select` selection shared one entry. A daemon that answered a cache decision before someone ran `sudo xcode-select -s` kept answering with the Xcode the host had left, and because every hit re-set the entry, a busy daemon served it indefinitely rather than for one TTL (#3300). - Resolve the default selection per read and key on it, keeping a client's `DEVELOPER_DIR` choice as before. - Run that resolution under the probe clock and cancellation, capped below the probes it precedes. A read that cannot resolve its selection skips the memo: a key shared by every unresolved selection would let one host selection answer for the next. - Measure the TTL from the read that stored an entry; a hit no longer extends it. - Keep the report path probe-free by looking entries up under the dir the read that stored one resolved. Related: #3449, whose Xcode-too-old reports should name the toolchain a decision actually read. * fix(apple): point the runner failure report at the selection a hit resolved A default-selection read that hit the memo returned before updating the report pointer, so after `xcode-select` moved A to B and back to A, a failure report named B for an A-backed runner. Route the read and the hit through one memo lookup so both record the dir the read resolved, and say so in that pointer's comment. Also drop "or skip" from the value-probe guard's comment: that guard only ever throws. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * fix(apple): say what a runner failure report guarantees about its entry The recorded selection outlives the entry it names: the memo's TTL is measured from the read that stored the entry, so a report can look it up after that entry is gone. Say that, and pin the case with a test that a report then names no Xcode rather than another selection's. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * fix(apple): key the runner toolchain memo on the selection's real dir The reviewer's xcodes case: a tool swaps the Xcode bundle behind a stable path, so `xcode-select -p` answers the same string and the key never moves. Resolve the host's selection link to the dir it really names, and follow the same path for a client's DEVELOPER_DIR so one toolchain lands on one entry. The link is read first, so a warm read costs no spawn; `xcode-select -p` is the fallback. Restore TTL renewal on a hit, which #3300 asked to keep: the key now moves with the selection, so a hit is evidence the entry still answers for this host. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * fix(apple): key the runner toolchain memo on the bundle standing in the dir A hit renews its entry, so a key that sees only the dir pins a stale fingerprint for as long as a daemon stays busy when an update replaces the bundle in place: an installer or App Store update at /Applications/Xcode.app keeps the path. Add the inode and mtime of the version file xcodebuild reads to the key, so the replaced bundle answers under a new key and the old entry expires once nothing renews it. The failure report now reads only the memo. It reused the key halves a real read built instead of resolving and stating the selection itself, which is what lets a hit renew at all: a report has no budget to spend and no deadline to answer to. Cancellation is observed after the key's two filesystem reads, so a warm hit cannot hide an abort. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * refactor(apple): own the selection link path beside the probe ceiling The reviewer's nit: it was exported only so the suite could match it. It belongs with the other Apple toolchain probe constant both probers read, where it can be named without that. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * fix(apple): keep each client's toolchain answer for its own failure report One global record meant the last client-named DEVELOPER_DIR read erased what an earlier client needed to report on, even while that client's fingerprint was still cached. A daemon serves request-scoped selections concurrently, so retain the answer per name instead. It is a TTL memo, so a name whose reads stopped drops out on the same clock as the entry it names. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * test(apple): pin the replacement bundle's mtime so the key must move The in-place replacement case relied on the filesystem to make both halves of the memo key differ. On ext4 or tmpfs a freed inode is reused and two writes can land in one truncated millisecond, leaving the key untouched and the case red. Set the replacement's mtime explicitly. Also drop the claim that two probers read the selection link path; only the toolchain probe does. --------- Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
1 parent f6fc99f commit 620228b

5 files changed

Lines changed: 980 additions & 84 deletions

File tree

‎packages/platform-apple/src/runner/__tests__/apple-toolchain-fixtures.ts‎

Lines changed: 95 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import { beforeEach, vi } from 'vitest';
2+
import fs from 'node:fs';
3+
import { AppError } from '@agent-device/kernel/errors';
24
import { appleRunnerTestHost } from '../test-host.ts';
3-
import type { ExecResult } from '@agent-device/host-kit/command';
5+
import { XCODE_SELECT_LINK_PATH } from '../apple-runner-platform.ts';
6+
import type { ExecOptions, ExecResult } from '@agent-device/host-kit/command';
47

58
export const STUBBED_APPLE_TOOLCHAIN = {
69
xcodeVersion: '26.2',
@@ -9,7 +12,16 @@ export const STUBBED_APPLE_TOOLCHAIN = {
912
sdkBuildVersion: '23C53',
1013
} as const;
1114

15+
const STUBBED_SELECTED_DEVELOPER_DIR = '/Applications/Xcode-Stub.app/Contents/Developer';
16+
17+
export function selectedDeveloperDirResult(developerDir: string): ExecResult {
18+
return { exitCode: 0, stdout: `${developerDir}\n`, stderr: '' };
19+
}
20+
1221
export function appleToolchainProbeResult(command: string, args: readonly string[]): ExecResult {
22+
if (command === 'xcode-select' && args[0] === '-p') {
23+
return selectedDeveloperDirResult(STUBBED_SELECTED_DEVELOPER_DIR);
24+
}
1325
if (command === 'xcodebuild' && args[0] === '-version') {
1426
return {
1527
exitCode: 0,
@@ -27,15 +39,93 @@ export function appleToolchainProbeResult(command: string, args: readonly string
2739
}
2840

2941
/**
30-
* Answers the runner cache's toolchain probes from a fixed toolchain, so cases
31-
* that key the cache neither read the host's Xcode nor depend on one existing.
32-
* Returns the mock so a case can make a probe fail.
42+
* Answers the runner cache's toolchain probes from a fixed toolchain, so cases that key the cache
43+
* neither read the host's Xcode nor depend on one existing. No client chooses a `DEVELOPER_DIR`, so
44+
* the selection a key probe resolves is the one the case scripts. Returns the mock so a case can
45+
* make a probe fail.
3346
*/
3447
export function stubAppleToolchainProbes(): ReturnType<typeof vi.fn> {
48+
stubAppleSelectionLink();
3549
const runCmdSync = vi.fn(appleToolchainProbeResult);
3650
beforeEach(() => {
3751
runCmdSync.mockImplementation(appleToolchainProbeResult);
38-
appleRunnerTestHost.update({ runCmdSync });
52+
appleRunnerTestHost.update({
53+
runCmdSync,
54+
commandDeveloperDir: () => undefined,
55+
});
3956
});
4057
return runCmdSync;
4158
}
59+
60+
/**
61+
* The host's default selection link as the Apple toolchain probes see it. Undefined keeps the link
62+
* unreadable, which sends every read down the `xcode-select -p` fallback a case already scripts; a
63+
* case that owns a link points it at a path it created.
64+
*/
65+
export const appleSelectionLink: { target: string | undefined } = { target: undefined };
66+
67+
/**
68+
* Hides the selection link of the machine running the suite, so a case decides the host's selection
69+
* rather than inheriting it. The real host has one, and where it points is not this suite's business.
70+
*/
71+
export function stubAppleSelectionLink(): void {
72+
const realRealpathSync = fs.realpathSync;
73+
beforeEach(() => {
74+
appleSelectionLink.target = undefined;
75+
vi.spyOn(fs, 'realpathSync').mockImplementation((target: fs.PathLike) => {
76+
if (String(target) !== XCODE_SELECT_LINK_PATH) return realRealpathSync(target);
77+
if (appleSelectionLink.target === undefined) {
78+
throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT' });
79+
}
80+
return realRealpathSync(appleSelectionLink.target);
81+
});
82+
});
83+
}
84+
85+
/**
86+
* The fingerprint budget's clock, advanced only by an exec that blocks for the whole timeout it was
87+
* handed. Without it a mock that throws immediately proves nothing about a deadline: no time passes,
88+
* so every budget looks untouched however many attempts run.
89+
*/
90+
export function installToolchainProbeClock(): { nowMs: number } {
91+
const clock = { nowMs: 0 };
92+
appleRunnerTestHost.update({
93+
deadlineFromTimeoutMs: (timeoutMs: number) => {
94+
const startedAtMs = clock.nowMs;
95+
const expiresAtMs = startedAtMs + Math.max(0, timeoutMs);
96+
return {
97+
remainingMs: () => Math.max(0, expiresAtMs - clock.nowMs),
98+
elapsedMs: () => Math.max(0, clock.nowMs - startedAtMs),
99+
isExpired: () => expiresAtMs - clock.nowMs <= 0,
100+
};
101+
},
102+
});
103+
return clock;
104+
}
105+
106+
/** An exec that blocked for its whole timeout and was then killed, as the exec layer reports it. */
107+
export function blockForWholeTimeout(
108+
clock: { nowMs: number },
109+
command: string,
110+
args: readonly string[],
111+
options: ExecOptions,
112+
): AppError {
113+
const timeoutMs = options.timeoutMs ?? 0;
114+
clock.nowMs += timeoutMs;
115+
return new AppError('COMMAND_FAILED', `${command} timed out after ${timeoutMs}ms`, {
116+
cmd: command,
117+
args,
118+
timeoutMs,
119+
});
120+
}
121+
122+
/**
123+
* The execs that read the toolchain itself, as opposed to the `xcode-select -p` a read runs to
124+
* decide which fingerprint entry it belongs to. A case that says a cache hit must not re-read the
125+
* toolchain says nothing about that one exec, so it asserts through this.
126+
*/
127+
export function valueProbeCommands(calls: readonly (readonly unknown[])[]): string[] {
128+
return calls
129+
.filter(([command]) => command !== 'xcode-select')
130+
.map(([command]) => String(command));
131+
}

‎packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts‎

Lines changed: 58 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,12 @@ import { memoizedRunnerXcodeVersion } from '../runner-toolchain-probe.ts';
2323
import { COLD_TOOLCHAIN_PROBE_TIMEOUT_MS } from '../apple-runner-platform.ts';
2424
import {
2525
appleToolchainProbeResult,
26+
blockForWholeTimeout,
27+
installToolchainProbeClock,
28+
selectedDeveloperDirResult,
2629
STUBBED_APPLE_TOOLCHAIN,
2730
stubAppleToolchainProbes,
31+
valueProbeCommands,
2832
} from './apple-toolchain-fixtures.ts';
2933
import { mkdtempForTestSync } from './tmp-dir.ts';
3034
import { xcodebuildLogWithBuildArguments } from './runner-build-log.fixtures.ts';
@@ -272,7 +276,7 @@ describe('toolchain probe budget', () => {
272276
beforeEach(resetAllProcessMemosForTests);
273277

274278
test('a cold-start probe recovers on retry, and the stall it survived is charged to the budget', () => {
275-
const clock = installFakeToolchainClock();
279+
const clock = installToolchainProbeClock();
276280
const xcodebuildTimeouts: number[] = [];
277281
runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => {
278282
if (command !== 'xcodebuild') return appleToolchainProbeResult(command, args);
@@ -292,9 +296,10 @@ describe('toolchain probe budget', () => {
292296
});
293297

294298
test('an owning phase with 4 s left gets one 4 s attempt and no retry', () => {
295-
const clock = installFakeToolchainClock();
299+
const clock = installToolchainProbeClock();
296300
const phaseBudget = createRunnerPhaseBudget(4_000, undefined);
297301
runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => {
302+
if (command === 'xcode-select') return appleToolchainProbeResult(command, args);
298303
throw blockForWholeTimeout(clock, command, args, options);
299304
});
300305
runCmdSync.mockClear();
@@ -303,14 +308,14 @@ describe('toolchain probe budget', () => {
303308
() => resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, phaseBudget),
304309
(error: unknown) => expectRunnerPhaseBudgetExhausted(error),
305310
);
306-
assert.equal(runCmdSync.mock.calls.length, 1);
311+
assert.equal(valueProbeCommands(runCmdSync.mock.calls).length, 1);
307312
// The one attempt was capped by the phase, not by the 30 s per-call ceiling.
308-
assert.equal(runCmdSync.mock.calls[0]?.[2]?.timeoutMs, 4_000);
313+
assert.equal(runCmdSync.mock.calls.at(-1)?.[2]?.timeoutMs, 4_000);
309314
assert.equal(clock.nowMs, 4_000);
310315
});
311316

312317
test('a probe that failed on its own and merely says "timed out" in its message is not retried', () => {
313-
installFakeToolchainClock();
318+
installToolchainProbeClock();
314319
runCmdSync.mockImplementation((command: string, args: string[]) => {
315320
if (command !== 'xcodebuild') return appleToolchainProbeResult(command, args);
316321
// No `timeoutMs` detail: this is the tool reporting its own failure, not
@@ -439,7 +444,7 @@ describe('toolchain probe budget', () => {
439444
];
440445

441446
test.each(CANCELLATION_CASES)('cancellation matrix: $label', (testCase) => {
442-
const clock = installFakeToolchainClock();
447+
const clock = installToolchainProbeClock();
443448
const request = new AbortController();
444449
if (testCase.fingerprintCache === 'warm') {
445450
runCmdSync.mockImplementation(appleToolchainProbeResult);
@@ -449,6 +454,9 @@ describe('toolchain probe budget', () => {
449454

450455
let execs = 0;
451456
runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => {
457+
// The cache-key resolution picks which entry the read belongs to; it is never one of the
458+
// three probes this matrix interrupts.
459+
if (command === 'xcode-select') return appleToolchainProbeResult(command, args);
452460
execs += 1;
453461
if (!failsOnExec(testCase, execs)) return appleToolchainProbeResult(command, args);
454462
if (testCase.aborts === 'while-it-blocks') request.abort();
@@ -513,44 +521,6 @@ function expectRunnerPhaseBudgetExhausted(error: unknown): boolean {
513521
return true;
514522
}
515523

516-
/**
517-
* A clock the probes' own budget reads, advanced only by
518-
* {@link blockForWholeTimeout}. Without it a mock that throws immediately
519-
* proves nothing about a deadline: no time passes, so every budget looks
520-
* untouched however many attempts run.
521-
*/
522-
function installFakeToolchainClock(): { nowMs: number } {
523-
const clock = { nowMs: 0 };
524-
appleRunnerTestHost.update({
525-
deadlineFromTimeoutMs: (timeoutMs: number) => {
526-
const startedAtMs = clock.nowMs;
527-
const expiresAtMs = startedAtMs + Math.max(0, timeoutMs);
528-
return {
529-
remainingMs: () => Math.max(0, expiresAtMs - clock.nowMs),
530-
elapsedMs: () => Math.max(0, clock.nowMs - startedAtMs),
531-
isExpired: () => expiresAtMs - clock.nowMs <= 0,
532-
};
533-
},
534-
});
535-
return clock;
536-
}
537-
538-
/** A probe that blocked for its whole timeout and was then killed, as the exec layer reports it. */
539-
function blockForWholeTimeout(
540-
clock: { nowMs: number },
541-
command: string,
542-
args: string[],
543-
options: ExecOptions,
544-
): AppError {
545-
const timeoutMs = options.timeoutMs ?? 0;
546-
clock.nowMs += timeoutMs;
547-
return new AppError('COMMAND_FAILED', `${command} timed out after ${timeoutMs}ms`, {
548-
cmd: command,
549-
args,
550-
timeoutMs,
551-
});
552-
}
553-
554524
test('a failing probe reports its exit status rather than a fabricated SDK version', () => {
555525
runCmdSync.mockImplementation((command: string, args: readonly string[]) =>
556526
command === 'xcrun'
@@ -652,16 +622,14 @@ test('a malformed xcodebuild answer is not memoized: the next request re-probes
652622
assert.deepEqual(unavailableProbes(), [
653623
{ probe: 'xcodebuild -version', reason: 'unparsable_output' },
654624
]);
655-
const probeCallsWhileMalformed = runCmdSync.mock.calls.length;
656625

657626
runCmdSync.mockImplementation(appleToolchainProbeResult);
627+
runCmdSync.mockClear();
658628
const metadata = resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
659629

660630
assert.equal(metadata.xcodeVersion, '26.2');
661631
assert.equal(metadata.xcodeBuildVersion, '17C52');
662-
expect(runCmdSync.mock.calls.slice(probeCallsWhileMalformed).map(([command]) => command)).toEqual(
663-
['xcodebuild', 'xcrun', 'xcrun'],
664-
);
632+
expect(valueProbeCommands(runCmdSync.mock.calls)).toEqual(['xcodebuild', 'xcrun', 'xcrun']);
665633
});
666634

667635
test('only a complete, parsed toolchain fingerprint is memoized', () => {
@@ -681,15 +649,11 @@ test('only a complete, parsed toolchain fingerprint is memoized', () => {
681649
runCmdSync.mockClear();
682650
const first = resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
683651
// The healthy xcodebuild answer from the failed round was not kept either: all three re-run.
684-
expect(runCmdSync.mock.calls.map(([command]) => command)).toEqual([
685-
'xcodebuild',
686-
'xcrun',
687-
'xcrun',
688-
]);
652+
expect(valueProbeCommands(runCmdSync.mock.calls)).toEqual(['xcodebuild', 'xcrun', 'xcrun']);
689653

690654
runCmdSync.mockClear();
691655
const second = resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
692-
expect(runCmdSync).not.toHaveBeenCalled();
656+
expect(valueProbeCommands(runCmdSync.mock.calls)).toEqual([]);
693657
assert.deepEqual(
694658
[second.xcodeVersion, second.xcodeBuildVersion, second.sdkVersion, second.sdkBuildVersion],
695659
[first.xcodeVersion, first.xcodeBuildVersion, first.sdkVersion, first.sdkBuildVersion],
@@ -793,26 +757,60 @@ test('a toolchain read under one DEVELOPER_DIR does not answer for another, so i
793757
resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR).xcodeBuildVersion,
794758
STUBBED_APPLE_TOOLCHAIN.xcodeBuildVersion,
795759
);
796-
expect(runCmdSync).not.toHaveBeenCalled();
760+
expect(valueProbeCommands(runCmdSync.mock.calls)).toEqual([]);
797761
});
798762

799-
test('a toolchain fingerprint expires once no request uses it; each use renews it', () => {
763+
test('a default selection the host moved does not answer from the fingerprint it left behind', () => {
764+
// Same consequence as the DEVELOPER_DIR case above, reached through `xcode-select` instead:
765+
// `sudo xcode-select -s` changes what a read without a client dir is answering for (#3300).
766+
withoutRunnerDerivedPathEnv();
767+
let selectedDir = '/Applications/Xcode-A.app/Contents/Developer';
768+
runCmdSync.mockImplementation((command: string, args: readonly string[]) =>
769+
command === 'xcode-select'
770+
? selectedDeveloperDirResult(selectedDir)
771+
: command === 'xcodebuild' && selectedDir === '/Applications/Xcode-B.app/Contents/Developer'
772+
? { exitCode: 0, stdout: 'Xcode 27.0\nBuild version 18A100\n', stderr: '' }
773+
: appleToolchainProbeResult(command, args),
774+
);
775+
776+
const onA = resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
777+
selectedDir = '/Applications/Xcode-B.app/Contents/Developer';
778+
runCmdSync.mockClear();
779+
const onB = resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
780+
expect(runCmdSync.mock.calls.map(([command]) => command)).toEqual([
781+
'xcode-select',
782+
'xcodebuild',
783+
'xcrun',
784+
'xcrun',
785+
]);
786+
assert.equal(onA.xcodeBuildVersion, STUBBED_APPLE_TOOLCHAIN.xcodeBuildVersion);
787+
assert.equal(onB.xcodeBuildVersion, '18A100');
788+
// The derived path is what a retained runner is reused by, so a differing one makes it stale.
789+
assert.notEqual(
790+
resolveRunnerDerivedPath(IOS_SIMULATOR, onA),
791+
resolveRunnerDerivedPath(IOS_SIMULATOR, onB),
792+
);
793+
});
794+
795+
test('a toolchain fingerprint is renewed by each hit and expires once nothing uses it', () => {
800796
vi.useFakeTimers();
801797
try {
802798
appleRunnerTestHost.update({
803799
commandDeveloperDir: () => '/Applications/Xcode-A.app/Contents/Developer',
804800
});
805801
resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
806802
runCmdSync.mockClear();
807-
// Used every 9 minutes, it outlives a 10-minute window without another probe.
808-
vi.advanceTimersByTime(9 * 60_000);
809-
resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
803+
// A hit is an entry keyed on the resolved dir AND the identity of the bundle standing in it, so
804+
// the toolchain behind that key is still the one in place and the entry renews itself (#3300
805+
// asked to keep this). A bundle replaced in place moves the identity, not just the dir; see
806+
// `a bundle replaced in place at the same path moves the key`. What an idle entry cannot
807+
// outlast is the TTL, measured from the hit that last renewed it.
810808
vi.advanceTimersByTime(9 * 60_000);
811809
resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
812810
expect(runCmdSync).not.toHaveBeenCalled();
813811
expect(memoizedRunnerXcodeVersion(IOS_SIMULATOR)).toBe(STUBBED_APPLE_TOOLCHAIN.xcodeVersion);
814812

815-
vi.advanceTimersByTime(10 * 60_000);
813+
vi.advanceTimersByTime(10 * 60_001);
816814
expect(memoizedRunnerXcodeVersion(IOS_SIMULATOR)).toBeUndefined();
817815
resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR);
818816
expect(runCmdSync.mock.calls.map(([command]) => command)).toEqual([

0 commit comments

Comments
 (0)