Skip to content

Commit 6aa2f85

Browse files
thymikeethymikee
committed
refactor: the snapshot bridge cache runs under withProcessLock too
`ensureSnapshotBridgeBinary` was the one lock-and-run site left releasing in a `finally`, so a build that failed could still be reported as the lock it could not hand back. It goes through `withProcessLock` now, with the lock still taken through the snapshot-source host seam. The CHANGELOG names it in place of "atomic file publishes", which no site in this change was. The runner cache lock loses the same never-overridden wrapper the device-set lock lost, and the malformed-record case in the process-lock tests joins the list of uninformative records it was a copy of.
1 parent 876f746 commit 6aa2f85

4 files changed

Lines changed: 95 additions & 132 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@
160160
handed back, so a build or a publish that failed keeps its own error instead of being replaced by
161161
`Timed out waiting for …`, and work that succeeded still reports the lock it could not give back.
162162
The Apple runner's artifact, cache, lease and disposal paths, the managed-allocation store, the
163-
device-claim store, atomic file publishes, the Swift recording cache and the agent-browser setup
163+
device-claim store, the iOS snapshot bridge cache, the Swift recording cache and the agent-browser setup
164164
moved onto it, replacing hand-written try/catch pairs that each chose differently.
165165
- Fixed: the redirect of `~/Library/Developer/XCTestDevices` gives itself back in one order — restore
166166
the host's own device set, then release the lock — and a restore that was refused is what the caller

‎packages/host-kit/src/internal/process-lock.test.ts‎

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -203,29 +203,6 @@ test('a reacquired lock publishes a claim that its predecessor cannot reuse', as
203203
assert.notEqual(firstToken, secondToken);
204204
});
205205

206-
test('acquireProcessLock does not evict a live owner whose owner.json is malformed', async () => {
207-
const lockDirPath = path.join(tmpDir, 'malformed.lock');
208-
fs.mkdirSync(lockDirPath);
209-
fs.writeFileSync(path.join(lockDirPath, 'owner.json'), '{ pid: ');
210-
stampDirectoryAbandoned(lockDirPath);
211-
212-
await assert.rejects(
213-
() =>
214-
acquireProcessLock({
215-
lockDirPath,
216-
owner: currentProcessOwner(),
217-
timeoutMs: 50,
218-
pollMs: 1,
219-
}),
220-
(error: unknown) => {
221-
assert.ok(error instanceof AppError);
222-
assert.equal(error.details?.ownerRecordUnreadable, true);
223-
return true;
224-
},
225-
);
226-
assert.equal(fs.existsSync(lockDirPath), true);
227-
});
228-
229206
test('acquireProcessLock does not evict an owner record it cannot read', async () => {
230207
const lockDirPath = path.join(tmpDir, 'unreadable.lock');
231208
fs.mkdirSync(lockDirPath);
@@ -311,6 +288,7 @@ function listReclaimSiblings(directory: string): string[] {
311288
}
312289

313290
const UNINFORMATIVE_OWNER_RECORDS = [
291+
'{ pid: ',
314292
'null',
315293
'"999999999"',
316294
'{"pid":"999999999","startTime":null,"acquiredAtMs":1}',

‎packages/platform-apple/src/runner/runner-cache.ts‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import {
66
readProcessStartTime,
77
acquireProcessLock,
88
withProcessLock,
9-
type ProcessLockOwner,
109
isEnvTruthy,
1110
findProjectRoot,
1211
} from './host.ts';
@@ -119,30 +118,17 @@ export async function markRunnerXctestrunArtifactBadForRun(
119118
export async function acquireRunnerXctestrunCacheLock(
120119
derived: string,
121120
): Promise<() => Promise<void>> {
122-
return await acquireRunnerCacheProcessLock({
121+
return await acquireProcessLock({
123122
lockDirPath: resolveRunnerXctestrunCacheLockPath(derived),
124123
owner: {
125124
pid: process.pid,
126125
startTime: readProcessStartTime(process.pid),
127126
acquiredAtMs: Date.now(),
128127
},
129-
description: 'iOS runner cache lock',
130-
});
131-
}
132-
133-
async function acquireRunnerCacheProcessLock(params: {
134-
lockDirPath: string;
135-
owner: ProcessLockOwner;
136-
timeoutMs?: number;
137-
description?: string;
138-
}): Promise<() => Promise<void>> {
139-
return await acquireProcessLock({
140-
lockDirPath: params.lockDirPath,
141-
owner: params.owner,
142-
timeoutMs: params.timeoutMs ?? RUNNER_XCTESTRUN_CACHE_LOCK_TIMEOUT_MS,
128+
timeoutMs: RUNNER_XCTESTRUN_CACHE_LOCK_TIMEOUT_MS,
143129
pollMs: RUNNER_XCTESTRUN_CACHE_LOCK_POLL_MS,
144130
ownerGraceMs: RUNNER_XCTESTRUN_CACHE_LOCK_OWNER_GRACE_MS,
145-
description: params.description ?? 'iOS runner cache lock',
131+
description: 'iOS runner cache lock',
146132
});
147133
}
148134

‎packages/platform-apple/src/snapshot-source/cache.ts‎

Lines changed: 90 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { createHash } from 'node:crypto';
22
import path from 'node:path';
3+
import { withProcessLock } from '@agent-device/host-kit/file';
34
import { SnapshotSourceError, snapshotSourceError } from './errors.ts';
45
import { remainingSnapshotSourceMs, type SnapshotSourceDeadline } from './deadline.ts';
56
import {
@@ -61,103 +62,101 @@ export async function ensureSnapshotBridgeBinary(
6162
const cacheRoot =
6263
input.cacheRoot ?? path.join(input.host.homeDirectory(), '.agent-device', 'snapshot-source');
6364
const entryPath = path.join(cacheRoot, cacheKey);
64-
const releaseLock = await input.host.acquireLock(path.join(cacheRoot, `${cacheKey}.lock`), {
65-
deadline,
66-
});
67-
try {
68-
const cached = await readValidCache(
69-
input.host,
70-
entryPath,
71-
{
72-
sourceHash,
73-
cacheKey,
74-
toolchain,
75-
},
76-
deadline,
77-
);
78-
if (cached) return cached;
79-
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
80-
if (input.host.exists(entryPath)) await input.host.remove(entryPath);
81-
82-
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
83-
await input.host.ensureDirectory(cacheRoot);
84-
const temporaryPath = path.join(cacheRoot, `.${cacheKey}.${input.host.processId()}.tmp`);
85-
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
86-
await input.host.remove(temporaryPath);
87-
try {
88-
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
89-
await input.host.ensureDirectory(temporaryPath);
90-
const outputPath = path.join(temporaryPath, BRIDGE_FILENAME);
91-
const result = await input.host.run(
92-
'xcrun',
93-
[
94-
'--sdk',
95-
'iphonesimulator',
96-
'clang',
97-
'-arch',
98-
toolchain.architecture,
99-
'-mios-simulator-version-min=15.0',
100-
'-fobjc-arc',
101-
'-Werror',
102-
'-Wall',
103-
'-Wextra',
104-
'-framework',
105-
'Foundation',
106-
'-framework',
107-
'CoreGraphics',
108-
...SNAPSHOT_BRIDGE_COMPILE_FILENAMES.map((sourceFile) =>
109-
path.join(sourceRoot, sourceFile),
110-
),
111-
'-o',
112-
outputPath,
113-
],
65+
return await withProcessLock({
66+
acquire: () => input.host.acquireLock(path.join(cacheRoot, `${cacheKey}.lock`), { deadline }),
67+
task: async () => {
68+
const cached = await readValidCache(
69+
input.host,
70+
entryPath,
11471
{
115-
signal: deadline.signal,
116-
timeoutMs: Math.min(
117-
BUILD_TIMEOUT_MS,
118-
remainingSnapshotSourceMs(deadline, 'native-build-deadline'),
119-
),
120-
allowFailure: true,
72+
sourceHash,
73+
cacheKey,
74+
toolchain,
12175
},
76+
deadline,
12277
);
123-
if (result.exitCode !== 0 || !input.host.exists(outputPath)) {
124-
throw snapshotSourceError('unsupported', 'native-build-failed', {
125-
exitCode: result.exitCode,
126-
stderr: result.stderr.slice(0, 4096),
127-
});
128-
}
78+
if (cached) return cached;
12979
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
130-
await input.host.chmod(outputPath, 0o755);
131-
const binarySha256 = await sha256File(input.host, outputPath, deadline);
132-
const manifest: SnapshotBridgeCacheManifest = {
133-
schemaVersion: CACHE_SCHEMA_VERSION,
134-
protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION,
135-
sourceVersion: SNAPSHOT_SOURCE_VERSION,
136-
sourceHash,
137-
cacheKey,
138-
toolchain,
139-
binarySha256,
140-
};
141-
await input.host.writeText(
142-
path.join(temporaryPath, MANIFEST_FILENAME),
143-
`${JSON.stringify(manifest, null, 2)}\n`,
144-
);
80+
if (input.host.exists(entryPath)) await input.host.remove(entryPath);
81+
82+
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
83+
await input.host.ensureDirectory(cacheRoot);
84+
const temporaryPath = path.join(cacheRoot, `.${cacheKey}.${input.host.processId()}.tmp`);
14585
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
146-
await input.host.rename(temporaryPath, entryPath);
147-
return {
148-
path: path.join(entryPath, BRIDGE_FILENAME),
149-
sourceHash,
150-
cacheKey,
151-
protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION,
152-
sourceVersion: SNAPSHOT_SOURCE_VERSION,
153-
};
154-
} catch (error) {
15586
await input.host.remove(temporaryPath);
156-
throw error;
157-
}
158-
} finally {
159-
await releaseLock();
160-
}
87+
try {
88+
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
89+
await input.host.ensureDirectory(temporaryPath);
90+
const outputPath = path.join(temporaryPath, BRIDGE_FILENAME);
91+
const result = await input.host.run(
92+
'xcrun',
93+
[
94+
'--sdk',
95+
'iphonesimulator',
96+
'clang',
97+
'-arch',
98+
toolchain.architecture,
99+
'-mios-simulator-version-min=15.0',
100+
'-fobjc-arc',
101+
'-Werror',
102+
'-Wall',
103+
'-Wextra',
104+
'-framework',
105+
'Foundation',
106+
'-framework',
107+
'CoreGraphics',
108+
...SNAPSHOT_BRIDGE_COMPILE_FILENAMES.map((sourceFile) =>
109+
path.join(sourceRoot, sourceFile),
110+
),
111+
'-o',
112+
outputPath,
113+
],
114+
{
115+
signal: deadline.signal,
116+
timeoutMs: Math.min(
117+
BUILD_TIMEOUT_MS,
118+
remainingSnapshotSourceMs(deadline, 'native-build-deadline'),
119+
),
120+
allowFailure: true,
121+
},
122+
);
123+
if (result.exitCode !== 0 || !input.host.exists(outputPath)) {
124+
throw snapshotSourceError('unsupported', 'native-build-failed', {
125+
exitCode: result.exitCode,
126+
stderr: result.stderr.slice(0, 4096),
127+
});
128+
}
129+
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
130+
await input.host.chmod(outputPath, 0o755);
131+
const binarySha256 = await sha256File(input.host, outputPath, deadline);
132+
const manifest: SnapshotBridgeCacheManifest = {
133+
schemaVersion: CACHE_SCHEMA_VERSION,
134+
protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION,
135+
sourceVersion: SNAPSHOT_SOURCE_VERSION,
136+
sourceHash,
137+
cacheKey,
138+
toolchain,
139+
binarySha256,
140+
};
141+
await input.host.writeText(
142+
path.join(temporaryPath, MANIFEST_FILENAME),
143+
`${JSON.stringify(manifest, null, 2)}\n`,
144+
);
145+
remainingSnapshotSourceMs(deadline, 'native-build-deadline');
146+
await input.host.rename(temporaryPath, entryPath);
147+
return {
148+
path: path.join(entryPath, BRIDGE_FILENAME),
149+
sourceHash,
150+
cacheKey,
151+
protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION,
152+
sourceVersion: SNAPSHOT_SOURCE_VERSION,
153+
};
154+
} catch (error) {
155+
await input.host.remove(temporaryPath);
156+
throw error;
157+
}
158+
},
159+
});
161160
}
162161

163162
function resolveSnapshotBridgeSourceRoot(host: SnapshotSourceHost): string {

0 commit comments

Comments
 (0)