Skip to content

Commit 534d3f4

Browse files
committed
refactor(runner): one hand-back answers for all three redirect exits
The install-failure catch kept its own try/catch around the restore, so a restore that could not run reached only the log while the caller saw the redirect failure alone. It now uses the same ordered hand-back as the handle and the no-redirect exit, and carries the restore failure in the error's details with a hint naming the path the host's device set is waiting at.
1 parent 756fa0f commit 534d3f4

3 files changed

Lines changed: 117 additions & 60 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@
116116
is told, whichever give-back door a teardown used. The lock's own complaint used to replace it in a
117117
`finally` and the best-effort door then dropped it, leaving the symlink pointed at the agent-device
118118
simulator set with nothing said about why. A simulator whose set already is `XCTestDevices` is no
119-
longer failed by a lock it could not verify, either.
119+
longer failed by a lock it could not verify, either, and a redirect that could not be installed names
120+
the path a failed restore left the host's device set at instead of only logging it.
120121
- Changed (sessions): the implicit session is now keyed by workspace **and platform**, so one checkout
121122

122123
can drive iOS and Android without inventing a `--session` name for every command (#2580). An

‎packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@ const iosSimulator: DeviceInfo = {
2626
type RedirectPaths = {
2727
requestedSetPath: string;
2828
xctestDeviceSetPath: string;
29+
backupPath: string;
2930
lockDirPath: string;
3031
};
3132

3233
function makeRedirectPaths(root: string): RedirectPaths {
3334
return {
3435
requestedSetPath: path.join(root, 'requested'),
3536
xctestDeviceSetPath: path.join(root, 'Library', 'Developer', 'XCTestDevices'),
37+
backupPath: path.join(root, 'Library', 'Developer', 'XCTestDevices.set-aside'),
3638
lockDirPath: path.join(root, '.agent-device', 'xctest-device-set.lock'),
3739
};
3840
}
@@ -222,6 +224,47 @@ test('a restore that was refused outranks the lock that could not be verified',
222224
}
223225
});
224226

227+
test('the host’s device set is back before the lock is', async () => {
228+
await withTempDir('device-set-hand-back-order-', async (root) => {
229+
const paths = makeRedirectPaths(root);
230+
fs.mkdirSync(paths.requestedSetPath, { recursive: true });
231+
fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true });
232+
233+
// The lock is what lets another runner build read the host's `XCTestDevices`, so the order the
234+
// give-back works in is the contract: releasing first would hand out a directory that is still a
235+
// symlink into this simulator's set. The events record when each step actually happened.
236+
const events: string[] = [];
237+
const realRename = fs.renameSync;
238+
const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((from, to) => {
239+
const source = String(from);
240+
const target = String(to);
241+
if (target === paths.backupPath) events.push('renamed-aside');
242+
else if (source === paths.backupPath) events.push('restored');
243+
else if (target === paths.xctestDeviceSetPath) events.push('symlink-installed');
244+
return realRename(from, to);
245+
}) as typeof fs.renameSync);
246+
const realRemoveDir = fs.rmdirSync;
247+
const removeDirSpy = vi.spyOn(fs, 'rmdirSync').mockImplementation(((target, ...rest) => {
248+
if (String(target) === paths.lockDirPath) events.push('lock-released');
249+
return realRemoveDir(target, ...rest);
250+
}) as typeof fs.rmdirSync);
251+
252+
try {
253+
const handle = await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), {
254+
...redirectOptions(paths),
255+
backupPath: paths.backupPath,
256+
});
257+
assert.ok(handle);
258+
await handle.release();
259+
260+
assert.deepEqual(events, ['renamed-aside', 'symlink-installed', 'restored', 'lock-released']);
261+
} finally {
262+
renameSpy.mockRestore();
263+
removeDirSpy.mockRestore();
264+
}
265+
});
266+
});
267+
225268
test('a redirect that could not be installed reports the redirect, not the failed clean-up', async () => {
226269
await withTempDir('device-set-install-failure-', async (root) => {
227270
const paths = makeRedirectPaths(root);
@@ -230,7 +273,8 @@ test('a redirect that could not be installed reports the redirect, not the faile
230273
fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'the host owns this');
231274

232275
// The install breaks after the host's set is renamed into the backup, and the restore that the
233-
// catch runs to undo it breaks too. Two failures, one report: the redirect that did not happen.
276+
// catch runs to undo it breaks too. Two failures, one report: the redirect that did not happen,
277+
// carrying the restore that could not run and naming where the host's device set is waiting.
234278
let restoreAttempted = false;
235279
const realRename = fs.renameSync;
236280
const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((
@@ -253,11 +297,17 @@ test('a redirect that could not be installed reports the redirect, not the faile
253297
acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), {
254298
lockDirPath: paths.lockDirPath,
255299
xctestDeviceSetPath: paths.xctestDeviceSetPath,
300+
backupPath: paths.backupPath,
256301
}),
257302
(error: unknown) => {
258303
assert.ok(error instanceof AppError);
259304
assert.equal(error.message, 'Failed to redirect XCTest device set path');
260305
assert.match(String(error.details?.error), /EPERM/);
306+
assert.match(String(error.details?.restoreError), /EACCES/);
307+
assert.ok(
308+
String(error.details?.hint).includes(paths.backupPath),
309+
`the hint must name where the host's device set is: ${String(error.details?.hint)}`,
310+
);
261311
return true;
262312
},
263313
);

‎packages/platform-apple/src/runner/runner-device-set.ts‎

Lines changed: 64 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -104,28 +104,19 @@ export async function acquireXcodebuildSimulatorSetRedirect(
104104
},
105105
});
106106

107-
const giveLockBack = async (): Promise<unknown> => {
108-
try {
109-
await releaseLock();
110-
return null;
111-
} catch (error) {
112-
return error;
113-
}
114-
};
107+
const paths = { xctestDeviceSetPath, backupPath };
115108

116-
try {
117-
reconcileXcodebuildSimulatorSetRedirect({
118-
xctestDeviceSetPath,
119-
backupPath,
120-
});
121-
if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) {
122-
// Nothing was redirected, so nothing was displaced and this simulator needs no handle: the
123-
// caller has no redirect to report, and a lock it never held must not arrive as one. The
124-
// give-back records its own failure where the claim lives.
125-
recordHandBackFailure(await giveLockBack(), lockDirPath);
126-
return null;
127-
}
109+
if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) {
110+
// Nothing is displaced and the caller gets no handle, so this hand-back has no failure of its own
111+
// to report: a lock this simulator never needed must not arrive as a redirect problem. Whatever it
112+
// could not verify is recorded where the claim lives.
113+
recordReleaseFailure((await handBackDeviceSet(paths, releaseLock)).releaseFailure, lockDirPath);
114+
return null;
115+
}
128116

117+
try {
118+
// Clean up whatever an earlier run left, out of the way of the rename below.
119+
reconcileXcodebuildSimulatorSetRedirect(paths);
129120
fs.mkdirSync(requestedSetPath, { recursive: true });
130121
if (fs.existsSync(xctestDeviceSetPath)) {
131122
fs.renameSync(xctestDeviceSetPath, backupPath);
@@ -135,56 +126,37 @@ export async function acquireXcodebuildSimulatorSetRedirect(
135126
xctestDeviceSetPath,
136127
});
137128
} catch (error) {
138-
// The redirect failure outranks everything the hand-back does, including a restore that could not
139-
// run on the way out. Both are recorded; the caller hears why the redirect failed.
140-
try {
141-
reconcileXcodebuildSimulatorSetRedirect({
142-
xctestDeviceSetPath,
143-
backupPath,
144-
});
145-
} catch (restoreError) {
146-
emitDiagnostic({
147-
level: 'warn',
148-
phase: 'ios_runner_xctest_device_set_restore_failed',
149-
data: { xctestDeviceSetPath, backupPath, error: String(restoreError) },
150-
});
151-
}
152-
recordHandBackFailure(await giveLockBack(), lockDirPath);
129+
const handBack = await handBackDeviceSet(paths, releaseLock);
130+
recordReleaseFailure(handBack.releaseFailure, lockDirPath);
153131
throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', {
154132
requestedSetPath,
155133
xctestDeviceSetPath,
156134
backupPath,
157135
error: String(error),
136+
...(handBack.restoreFailure === null
137+
? {}
138+
: {
139+
restoreError: String(handBack.restoreFailure),
140+
hint:
141+
`The host's own device set is still renamed aside at ${backupPath}: restore it, ` +
142+
'or remove that path, before another runner build redirects it.',
143+
}),
158144
});
159145
}
160146

161147
let givenBack = false;
162-
// One ordered give-back: restore the host's own device set, then hand the lock back. A restore that
163-
// could not run is a fact about this machine — the symlink stays pointed at this simulator's set and
164-
// every later `simctl` run sees the wrong devices — so its failure always outranks the release, and
165-
// the lock goes back anyway so the next acquire does not wait on it. Whatever the release could not
166-
// do is recorded either way; the two doors differ only in whether the caller also throws it.
167148
const giveBack = async (reportUnverifiedRelease: boolean): Promise<void> => {
168149
if (givenBack) {
169150
return;
170151
}
171152
givenBack = true;
172-
let restoreFailure: unknown = null;
173-
try {
174-
reconcileXcodebuildSimulatorSetRedirect({
175-
xctestDeviceSetPath,
176-
backupPath,
177-
});
178-
} catch (error) {
179-
restoreFailure = error;
153+
const handBack = await handBackDeviceSet(paths, releaseLock);
154+
recordReleaseFailure(handBack.releaseFailure, lockDirPath);
155+
if (handBack.restoreFailure !== null) {
156+
throw handBack.restoreFailure;
180157
}
181-
const releaseFailure = await giveLockBack();
182-
recordHandBackFailure(releaseFailure, lockDirPath);
183-
if (restoreFailure !== null) {
184-
throw restoreFailure;
185-
}
186-
if (releaseFailure !== null && reportUnverifiedRelease) {
187-
throw releaseFailure;
158+
if (handBack.releaseFailure !== null && reportUnverifiedRelease) {
159+
throw handBack.releaseFailure;
188160
}
189161
};
190162
return {
@@ -193,11 +165,45 @@ export async function acquireXcodebuildSimulatorSetRedirect(
193165
};
194166
}
195167

168+
type DeviceSetHandBack = {
169+
/** The host's own `XCTestDevices` could not be put back, so the symlink is still in its place. */
170+
restoreFailure: unknown;
171+
/** The lock could not be given back, or could not be verified as ours when it was. */
172+
releaseFailure: unknown;
173+
};
174+
175+
/**
176+
* The one ordered hand-back, used by every path that leaves this redirect behind: restore the host's
177+
* own device set, then release the lock. Neither step can hide the other, because a restore that could
178+
* not run is a fact about this machine that outlives the request — every later `simctl` run sees the
179+
* wrong devices — and the lock going back is what keeps the next acquire from waiting on a claim nobody
180+
* is acting on. Deciding which of the two the caller hears is left to the caller that has a report to
181+
* make: the redirect failure, the restore failure, or neither.
182+
*/
183+
async function handBackDeviceSet(
184+
paths: { xctestDeviceSetPath: string; backupPath: string },
185+
releaseLock: () => Promise<void>,
186+
): Promise<DeviceSetHandBack> {
187+
let restoreFailure: unknown = null;
188+
let releaseFailure: unknown = null;
189+
try {
190+
reconcileXcodebuildSimulatorSetRedirect(paths);
191+
} catch (error) {
192+
restoreFailure = error;
193+
}
194+
try {
195+
await releaseLock();
196+
} catch (error) {
197+
releaseFailure = error;
198+
}
199+
return { restoreFailure, releaseFailure };
200+
}
201+
196202
/**
197-
* Records a hand-back failure that no caller is being made to throw. A release whose ownership could
198-
* not be verified is already recorded where the claim lives, so only the rest reaches the log.
203+
* Records a lock release that no caller is being made to throw. A release whose ownership could not be
204+
* verified is already recorded where the claim lives, so only the rest reaches the log.
199205
*/
200-
function recordHandBackFailure(error: unknown, lockDirPath: string): void {
206+
function recordReleaseFailure(error: unknown, lockDirPath: string): void {
201207
if (error === null || isOwnerReleaseUnverified(error)) {
202208
return;
203209
}

0 commit comments

Comments
 (0)