Skip to content

Commit 39e4682

Browse files
authored
refactor: deepen runner disposal (#714)
* refactor: deepen runner disposal * docs: clarify PR descriptions
1 parent 49e59d6 commit 39e4682

7 files changed

Lines changed: 330 additions & 195 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,9 +295,9 @@ Command-only flags (like `find --first`) that do not flow to the platform layer
295295
- Open a ready-for-review PR by default. Use a draft PR only when the user explicitly asks for one or the work is intentionally incomplete.
296296
- Run required checks for touched scope from **Testing Matrix**.
297297
- PR body must be short and include:
298-
- `## Summary`: describe the user-visible or reviewer-relevant change as before/after when useful, and include the issue closed by the PR using `Closes #123` when applicable.
298+
- `## Summary`: lead with benefits and reviewer-relevant outcomes. Prefer a compact before/after when it makes the improvement clearer. Include the issue closed by the PR using `Closes #123` when applicable.
299299
- `## Validation`: answer this prompt in concise prose: "How did you verify the change, and what passed or changed on screen?" Prefer evidence over command dumps; mention the relevant check category or scenario, and include screenshots when visual/UI behavior is relevant.
300-
- Call out known gaps/follow-ups explicitly.
300+
- Call out real tradeoffs, known gaps, or follow-ups explicitly; omit boilerplate when there are none.
301301
- Include touched-file count and note if scope expanded beyond initial command family.
302302

303303
## Priority Order

src/daemon/__tests__/daemon-command-registry.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
shouldLockSessionExecution,
1414
shouldPreferExplicitDeviceOverExistingSession,
1515
shouldValidateSessionSelector,
16+
resolveProviderDeviceResolutionIntent,
1617
usesSessionlessDefaultProviderDevice,
1718
} from '../daemon-command-registry.ts';
1819
import type { DaemonRequest } from '../types.ts';
@@ -155,6 +156,40 @@ test('daemon command registry preserves provider device resolution traits', () =
155156
usesSessionlessDefaultProviderDevice(makeRequest(PUBLIC_COMMANDS.record, ['stop'])),
156157
false,
157158
);
159+
assert.equal(
160+
resolveProviderDeviceResolutionIntent(makeRequest(PUBLIC_COMMANDS.test), {
161+
hasExistingSession: false,
162+
hasExplicitDeviceSelector: false,
163+
}),
164+
'skip',
165+
);
166+
assert.equal(
167+
resolveProviderDeviceResolutionIntent(
168+
{
169+
...makeRequest(PUBLIC_COMMANDS.test),
170+
flags: { shardAll: 2 },
171+
},
172+
{
173+
hasExistingSession: false,
174+
hasExplicitDeviceSelector: true,
175+
},
176+
),
177+
'skip',
178+
);
179+
assert.equal(
180+
resolveProviderDeviceResolutionIntent(makeRequest(PUBLIC_COMMANDS.open), {
181+
hasExistingSession: false,
182+
hasExplicitDeviceSelector: false,
183+
}),
184+
'sessionless-default-device',
185+
);
186+
assert.equal(
187+
resolveProviderDeviceResolutionIntent(makeRequest(PUBLIC_COMMANDS.apps), {
188+
hasExistingSession: true,
189+
hasExplicitDeviceSelector: true,
190+
}),
191+
'explicit-device',
192+
);
158193
});
159194

160195
function makeRequest(command: string, positionals: string[] = []): DaemonRequest {

src/daemon/__tests__/request-platform-providers.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,39 @@ test('request platform provider scope follows explicit apps selector for existin
9292
assert.deepEqual(seenDevices, [`default:${OTHER_IOS_SIMULATOR.id}`]);
9393
});
9494

95+
test('request platform provider scope skips sharded test orchestration requests', async () => {
96+
let providerCalls = 0;
97+
98+
const result = await withTargetDeviceResolutionScope(
99+
async () => {
100+
throw new Error('Sharded test orchestration should not resolve a provider device');
101+
},
102+
async () =>
103+
await withRequestPlatformProviderScope(
104+
{
105+
req: {
106+
...request('test'),
107+
flags: {
108+
platform: 'ios',
109+
shardAll: 2,
110+
},
111+
},
112+
existingSession: undefined,
113+
providers: {
114+
appleToolProvider: () => {
115+
providerCalls += 1;
116+
return createLocalAppleToolProvider();
117+
},
118+
},
119+
},
120+
async () => 'unscoped',
121+
),
122+
);
123+
124+
assert.equal(result, 'unscoped');
125+
assert.equal(providerCalls, 0);
126+
});
127+
95128
test('request platform provider scopes stay isolated across concurrent requests', async () => {
96129
const androidCalls: string[] = [];
97130
const appleCalls: string[] = [];

src/daemon/daemon-command-registry.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,15 @@ type DaemonCommandDescriptor = {
2727
androidBlockingDialogGuard?: boolean;
2828
preferExplicitDeviceOverExistingSession?: boolean;
2929
allowSessionlessDefaultDevice?: (req: DaemonRequest) => boolean;
30+
skipSessionlessProviderDevice?: (req: DaemonRequest) => boolean;
3031
};
3132

33+
export type DaemonProviderDeviceResolutionIntent =
34+
| 'existing-session'
35+
| 'explicit-device'
36+
| 'sessionless-default-device'
37+
| 'skip';
38+
3239
const REQUEST_EXECUTION_EXEMPT = {
3340
leaseAdmissionExempt: true,
3441
sessionExecutionLockExempt: true,
@@ -78,7 +85,7 @@ const DAEMON_COMMAND_DESCRIPTORS = [
7885
),
7986
...descriptors(
8087
'session',
81-
{ sessionKind: 'replay' },
88+
{ sessionKind: 'replay', skipSessionlessProviderDevice: isShardedTestRequest },
8289
PUBLIC_COMMANDS.replay,
8390
PUBLIC_COMMANDS.test,
8491
),
@@ -212,6 +219,20 @@ export function usesSessionlessDefaultProviderDevice(req: DaemonRequest): boolea
212219
return typeof allow === 'function' ? allow(req) : false;
213220
}
214221

222+
export function resolveProviderDeviceResolutionIntent(
223+
req: DaemonRequest,
224+
params: { hasExistingSession: boolean; hasExplicitDeviceSelector: boolean },
225+
): DaemonProviderDeviceResolutionIntent {
226+
if (params.hasExistingSession) {
227+
return shouldPreferExplicitDeviceOverExistingSession(req) && params.hasExplicitDeviceSelector
228+
? 'explicit-device'
229+
: 'existing-session';
230+
}
231+
if (shouldSkipSessionlessProviderDevice(req)) return 'skip';
232+
if (params.hasExplicitDeviceSelector) return 'explicit-device';
233+
return usesSessionlessDefaultProviderDevice(req) ? 'sessionless-default-device' : 'skip';
234+
}
235+
215236
function descriptor(
216237
command: string,
217238
route: DaemonCommandRoute,
@@ -252,3 +273,15 @@ function buildDaemonCommandRegistry(descriptors: readonly DaemonCommandDescripto
252273
function isRecordStartRequest(req: DaemonRequest): boolean {
253274
return (req.positionals?.[0] ?? '').toLowerCase() === 'start';
254275
}
276+
277+
function shouldSkipSessionlessProviderDevice(req: DaemonRequest): boolean {
278+
const skip = getDaemonCommandDescriptor(req.command)?.skipSessionlessProviderDevice;
279+
return typeof skip === 'function' ? skip(req) : false;
280+
}
281+
282+
function isShardedTestRequest(req: DaemonRequest): boolean {
283+
return (
284+
req.command === PUBLIC_COMMANDS.test &&
285+
(typeof req.flags?.shardAll === 'number' || typeof req.flags?.shardSplit === 'number')
286+
);
287+
}

src/daemon/request-platform-providers.ts

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,7 @@ import type { AppLogProvider } from './app-log.ts';
1414
import { hasExplicitDeviceSelector } from './device-selector-intent.ts';
1515
import type { RecordingProvider } from './recording-provider.ts';
1616
import type { DaemonRequest, SessionState } from './types.ts';
17-
import {
18-
shouldPreferExplicitDeviceOverExistingSession,
19-
usesSessionlessDefaultProviderDevice,
20-
} from './daemon-command-registry.ts';
17+
import { resolveProviderDeviceResolutionIntent } from './daemon-command-registry.ts';
2118

2219
export type PlatformProviderRequestSession = Pick<
2320
SessionState,
@@ -279,35 +276,19 @@ async function resolveScopedProviderDevice(
279276
req: DaemonRequest,
280277
existingSession: SessionState | undefined,
281278
): Promise<DeviceInfo | undefined> {
282-
if (existingSession) {
283-
return await resolveExistingSessionProviderDevice(req, existingSession);
279+
const intent = resolveProviderDeviceResolutionIntent(req, {
280+
hasExistingSession: Boolean(existingSession),
281+
hasExplicitDeviceSelector: hasExplicitDeviceSelector(req.flags),
282+
});
283+
switch (intent) {
284+
case 'existing-session':
285+
return existingSession?.device;
286+
case 'explicit-device':
287+
case 'sessionless-default-device':
288+
return await resolveTargetDevice(req.flags ?? {});
289+
case 'skip':
290+
return undefined;
284291
}
285-
if (shouldSkipSessionlessProviderDevice(req)) return undefined;
286-
return await resolveTargetDevice(req.flags ?? {});
287-
}
288-
289-
async function resolveExistingSessionProviderDevice(
290-
req: DaemonRequest,
291-
existingSession: SessionState,
292-
): Promise<DeviceInfo> {
293-
if (!shouldResolveExplicitProviderDevice(req)) return existingSession.device;
294-
return await resolveTargetDevice(req.flags ?? {});
295-
}
296-
297-
function shouldResolveExplicitProviderDevice(req: DaemonRequest): boolean {
298-
return shouldPreferExplicitDeviceOverExistingSession(req) && hasExplicitDeviceSelector(req.flags);
299-
}
300-
301-
function shouldSkipSessionlessProviderDevice(req: DaemonRequest): boolean {
302-
if (isShardedReplayTestRequest(req)) return true;
303-
return !hasExplicitDeviceSelector(req.flags) && !usesSessionlessDefaultProviderDevice(req);
304-
}
305-
306-
function isShardedReplayTestRequest(req: DaemonRequest): boolean {
307-
return (
308-
req.command === 'test' &&
309-
(typeof req.flags?.shardAll === 'number' || typeof req.flags?.shardSplit === 'number')
310-
);
311292
}
312293

313294
async function requestPlatformProviderScopeWrappers(

0 commit comments

Comments
 (0)