Skip to content

test(apple): wait for the owner lookup's spawns by event, not a turn count #4244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thymikee wants to merge 1 commit into from
+17 −10
Closed

test(apple): wait for the owner lookup's spawns by event, not a turn count #4244

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions packages/platform-apple/src/launch-confirmation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,17 +218,20 @@
*/
function wedgedCoreSimulator(
answers: Readonly<Record<string, string>> = {},
onSpawn?: (count: number) => void,
): AppleToolProvider & { spawns: () => number } {
let spawns = 0;
const answered = (key: string): ExecResult | undefined => {
const stdout = answers[key];
if (stdout === undefined) return undefined;
spawns += 1;
onSpawn?.(spawns);
return { stdout, stderr: '', exitCode: 0 } as ExecResult;
};
const hang = async (options?: ExecOptions): Promise<ExecResult> =>
await new Promise((_resolve, reject) => {
spawns += 1;
onSpawn?.(spawns);
if (options?.signal?.aborted) {
reject(options.signal.reason);
return;
Expand Down Expand Up @@ -257,14 +260,6 @@
};
}

/** Real I/O turns until the provider saw `count` spawns; bounded so a lookup that stops early fails here, not at the test timeout. */
async function untilSpawned(coreSimulator: { spawns: () => number }, count: number): Promise<void> {
for (let turn = 0; turn < 1000 && coreSimulator.spawns() < count; turn += 1) {
await new Promise((resolve) => setImmediate(resolve));
}
expect(coreSimulator.spawns()).toBe(count);
}

const LISTED_APP = JSON.stringify({
'com.example.app': {
ApplicationType: 'User',
Expand All @@ -288,7 +283,13 @@
vi.mocked(emitDiagnostic).mockClear();
try {
const { interactor, acceptAlert } = confirmationRunner();
const coreSimulator = wedgedCoreSimulator(answers);
let signalLookupStarted: () => void;
const lookupStarted = new Promise<void>((resolve) => {
signalLookupStarted = resolve;
});
const coreSimulator = wedgedCoreSimulator(answers, (count) => {
if (count === 2) signalLookupStarted();
});
let answered: unknown = 'pending';
const answer = withAppleToolProvider(coreSimulator, async () =>
answerSimulatorLaunchConfirmation(
Expand All @@ -297,10 +298,16 @@
Promise.resolve(interactor),
new AbortController().signal,
undefined,
).then((value) => (answered = value)),
);

await untilSpawned(coreSimulator, 2);
await Promise.race([

@cubic-dev-ai cubic-dev-ai Bot Nov 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This race has no bounded fallback when the lookup stalls before its second spawn. Because the lookup timeout is not armed until that spawn, a delayed filesystem operation can hold the test until Vitest’s global timeout instead of failing fast; retain a bounded pre-spawn fallback.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/launch-confirmation.test.ts, line 304:

<comment>This race has no bounded fallback when the lookup stalls before its second spawn. Because the lookup timeout is not armed until that spawn, a delayed filesystem operation can hold the test until Vitest’s global timeout instead of failing fast; retain a bounded pre-spawn fallback.</comment>

<file context>
@@ -300,7 +301,13 @@ test.each([
       );
 
-      await untilSpawned(coreSimulator, 2);
+      await Promise.race([
+        lookupStarted,
+        answer.then(() => {
</file context>

lookupStarted,
answer.then(() => {
throw new Error('Owner lookup completed before the second command started');
}),
]);

Check warning on line 309 in packages/platform-apple/src/launch-confirmation.test.ts

View check run for this annotation

apex-sentinel-staging / Apex Sentinel

minor: Lookup that never spawns the second command now hangs to the test timeout 

When the owner lookup never spawns the second command (`expected +0 to be 2`), both race branches stay pending and the test dies on the Vitest timeout with a generic message instead of naming the spawn count. The new fast-fail covers only the opposite case; the bounded turn loop that was removed existed to fail this one, as its doc comment stated.

### Also raised here

Also raised at this location by persona `javascript`: Race has no bound: a lookup that stalls without a second spawn hangs the worker — If the owner lookup spawns the app list and then stalls on something that is neither a spawn nor a completion (a pending fs stat, a lost await), neither arm of the race settles: `lookupStarted` never resolves and `answer` never settles, so the test hangs instead of failing. The worker runs with `vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'Date'] })` in effect (line 282), so Vitest's own test-timeout timer is faked too and never fires — the file's remaining assertions never execute and the run dies on the worker/pool timeout, the same unattributed-failure shape this file's Coverage-lane history is about. The removed `untilSpawned` stated this as its purpose: "bounded so a lookup that stops early fails here, not at the test timeout", and its final `expect(spawns()).toBe(count)` still ran on exhaustion. The new code only detects the early stop that _completes_ (the `answer.then(throw)` arm); the arm for a stalled lookup is gone. …

Also raised at this location by persona `senior-developer`: Removal of untilSpawned leaves an unbounded wait: a lookup that never spawns fails as a bare test timeout — If the owner lookup stalls before its second spawn — the first command never starts, or it starts and its hang() promise settles only on a timer the fake clock never advances — `Promise.race` never settles and the test dies on Vitest's generic test timeout instead of the named assertion the removed helper produced. The race's fast-fail arm covers only `answer` \…
Comment on lines 301 to +309

@apex-sentinel-staging apex-sentinel-staging Bot Nov 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor · Lookup that never spawns the second command now hangs to the test timeout

When the owner lookup never spawns the second command (expected +0 to be 2), both race branches stay pending and the test dies on the Vitest timeout with a generic message instead of naming the spawn count. The new fast-fail covers only the opposite case; the bounded turn loop that was removed existed to fail this one, as its doc comment stated.

Suggested change

Keep a bounded wait so the never-spawned case reports the spawn count: race lookupStarted against answer and a timeout that asserts coreSimulator.spawns(), so a CI flake fails with "expected 0 to be 2" rather than the harness timeout.

Details

Also raised here

Also raised at this location by persona javascript: Race has no bound: a lookup that stalls without a second spawn hangs the worker — If the owner lookup spawns the app list and then stalls on something that is neither a spawn nor a completion (a pending fs stat, a lost await), neither arm of the race settles: lookupStarted never resolves and answer never settles, so the test hangs instead of failing. The worker runs with vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'Date'] }) in effect (line 282), so Vitest's own test-timeout timer is faked too and never fires — the file's remaining assertions never execute and the run dies on the worker/pool timeout, the same unattributed-failure shape this file's Coverage-lane history is about. The removed untilSpawned stated this as its purpose: "bounded so a lookup that stops early fails here, not at the test timeout", and its final expect(spawns()).toBe(count) still ran on exhaustion. The new code only detects the early stop that completes (the answer.then(throw) arm); the arm for a stalled lookup is gone. …

Also raised at this location by persona senior-developer: Removal of untilSpawned leaves an unbounded wait: a lookup that never spawns fails as a bare test timeout — If the owner lookup stalls before its second spawn — the first command never starts, or it starts and its hang() promise settles only on a timer the fake clock never advances — Promise.race never settles and the test dies on Vitest's generic test timeout instead of the named assertion the removed helper produced. The race's fast-fail arm covers only answer settling; it cannot cover a lookup that neither spawns twice nor completes, and with toFake: ['setTimeout','clearTimeout','Date'] nothing can advance it. The removed docstring stated this invariant explicitly: "bounded so a lookup that stops early fails here, not at the test timeout" (old launch-confirmation.test.ts:260-266). Keep the bound by racing the event against a wall-clock deadline, or assert a named failure when neither arm wins.

persona: maintainability, javascript, senior-developer

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

expect(coreSimulator.spawns()).toBe(2);
await vi.advanceTimersByTimeAsync(URL_OWNER_LOOKUP_TIMEOUT_MS + 1);
expect(answered).toEqual({ outcome: 'unreadable', step: 'url-owner' });
await answer;
Expand Down
Loading