From 51e9351ed79ff55ad2e74bd91fa7f48a68cca63e Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 12 Jun 2026 11:10:15 -0700 Subject: [PATCH] fix(ios-qa): isolate E2E tests under --concurrent (3 real races) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ios-qa E2E file failed intermittently under `bun test --concurrent` (the eval harness default). Three distinct shared-state races, all fixed: 1. Shared pidfile: a module-level `workDir` reassigned in beforeEach was clobbered by parallel tests, so concurrent daemons collided on the same pidfile and the loser returned `already_running`. Each test now gets its own dir via makeWorkDir(). 2. process.env path globals: tests set GSTACK_IOS_AUDIT_PATH / _ATTEMPTS_PATH / _ALLOWLIST_PATH on the shared process env; concurrent tests stomped each other's audit/attempts destinations. Threaded auditPath/attemptsPath/allowlistPath through DaemonOptions (and mintForCaller) as explicit args — env is no longer load-bearing. 3. afterEach cleanup race: the per-test cleanup drained a shared dir array, so the first test to finish deleted still-running tests' workDirs mid-assertion. Moved to afterAll (cleans once, after all settle). Verified: 5/5 clean full-suite runs at --max-concurrency 15 (was intermittent); daemon unit suite 91/91; daemon source compiles. The paths default to the env-derived locations when options are omitted, so the production CLI path is unchanged. Co-Authored-By: Claude Fable 5 --- ios-qa/daemon/src/auth-mint.ts | 4 +++ ios-qa/daemon/src/index.ts | 21 ++++++++++-- test/skill-e2e-ios.test.ts | 61 ++++++++++++++++++++++------------ 3 files changed, 63 insertions(+), 23 deletions(-) diff --git a/ios-qa/daemon/src/auth-mint.ts b/ios-qa/daemon/src/auth-mint.ts index 53416df17..f5e24c6df 100644 --- a/ios-qa/daemon/src/auth-mint.ts +++ b/ios-qa/daemon/src/auth-mint.ts @@ -31,6 +31,7 @@ export async function mintForCaller(opts: { request: MintRequest; tokenStore: SessionTokenStore; allowlistPath?: string; + attemptsPath?: string; endpoint?: string; }): Promise { const allowlist = await loadAllowlist(opts.allowlistPath); @@ -42,6 +43,7 @@ export async function mintForCaller(opts: { rawIdentity: opts.callerIdentity, endpoint: opts.endpoint ?? '/auth/mint', reason: 'identity_not_allowed', + path: opts.attemptsPath, }); return { error: 'identity_not_allowed' }; } @@ -52,6 +54,7 @@ export async function mintForCaller(opts: { rawIdentity: opts.callerIdentity, endpoint: opts.endpoint ?? '/auth/mint', reason: 'capability_insufficient', + path: opts.attemptsPath, }); return { error: 'capability_insufficient' }; } @@ -73,6 +76,7 @@ export async function mintForCaller(opts: { rawIdentity: opts.callerIdentity, endpoint: opts.endpoint ?? '/auth/mint', reason: 'rate_limited', + path: opts.attemptsPath, }); return { error: 'rate_limited' }; } diff --git a/ios-qa/daemon/src/index.ts b/ios-qa/daemon/src/index.ts index abfe38be3..82628b136 100644 --- a/ios-qa/daemon/src/index.ts +++ b/ios-qa/daemon/src/index.ts @@ -30,6 +30,12 @@ interface DaemonOptions { tailnetSocketPath?: string; tailnetSessionTtlSeconds?: number; pidfilePath?: string; + // Explicit security-log + allowlist paths. Default to the env-var-derived + // locations (defaultAuditPath etc.) when omitted, but passing them lets + // concurrent test instances stay isolated without racing on process.env. + auditPath?: string; + attemptsPath?: string; + allowlistPath?: string; // Test injection tunnelProvider?: () => Promise; whoIsImpl?: (addr: string) => Promise<{ identity: string; raw: unknown }>; @@ -112,6 +118,9 @@ export async function startDaemon(opts: DaemonOptions): Promise whoIs(addr, opts.tailnetSocketPath)), }); }); @@ -172,6 +181,10 @@ interface HandlerCtx { res: ServerResponse; tokenStore: SessionTokenStore; getTunnel: () => Promise; + // Explicit security-log + allowlist paths (default to env-derived when undefined). + auditPath?: string; + attemptsPath?: string; + allowlistPath?: string; } function readBody(req: IncomingMessage, maxBytes = 1_048_576): Promise { @@ -274,7 +287,7 @@ interface TailnetCtx extends HandlerCtx { * Tailnet handler — locked allowlist + capability tiers. */ async function handleTailnet(ctx: TailnetCtx): Promise { - const { req, res, tokenStore, getTunnel, whoIsImpl } = ctx; + const { req, res, tokenStore, getTunnel, whoIsImpl, auditPath, attemptsPath, allowlistPath } = ctx; const url = parseUrl(req.url ?? '/'); const path = url.pathname ?? '/'; const method = req.method ?? 'GET'; @@ -304,6 +317,7 @@ async function handleTailnet(ctx: TailnetCtx): Promise { rawIdentity: peerAddr, endpoint: route, reason: 'whois_unparseable', + path: attemptsPath, }); sendJson(res, 502, { error: 'whois_failed', detail: (err as Error).message }); return; @@ -318,6 +332,8 @@ async function handleTailnet(ctx: TailnetCtx): Promise { request: parsed, tokenStore, endpoint: route, + allowlistPath, + attemptsPath, }); if ('error' in result) { @@ -338,6 +354,7 @@ async function handleTailnet(ctx: TailnetCtx): Promise { rawIdentity: token ? 'token:' + token.slice(0, 8) : 'no_token', endpoint: route, reason: validation.reason, + path: attemptsPath, }); const status = validation.reason === 'capability_insufficient' ? 403 : 401; sendJson(res, status, { error: validation.reason }); @@ -383,7 +400,7 @@ async function handleTailnet(ctx: TailnetCtx): Promise { capability: session.capability, request_id: req.headers['x-request-id']?.toString() ?? '-', status: upstream.status, - }); + }, auditPath); } res.writeHead(upstream.status, upstream.headers); diff --git a/test/skill-e2e-ios.test.ts b/test/skill-e2e-ios.test.ts index 70676d8ab..8d8f09c56 100644 --- a/test/skill-e2e-ios.test.ts +++ b/test/skill-e2e-ios.test.ts @@ -12,7 +12,7 @@ // daemon source (ios-qa/daemon/test/*). This file tests the agent-flow // boundary — what the /ios-qa skill orchestrates end-to-end. -import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { describe, test, expect, afterAll } from 'bun:test'; import { createServer, type Server, type IncomingMessage } from 'http'; import { mkdtempSync, rmSync, mkdirSync, writeFileSync, existsSync, readFileSync } from 'fs'; import { tmpdir } from 'os'; @@ -26,14 +26,27 @@ const HAS_DEVICE = process.env.GSTACK_HAS_IOS_DEVICE === '1'; const DEVICE_TOKEN = 'rotated-mock-bearer-token'; -let workDir: string; +// Per-test isolation under `bun test --concurrent`: a single module-level +// `workDir` reassigned in beforeEach is clobbered by parallel tests, so they +// collide on the same daemon pidfile (`already_running`) and stomp each +// other's GSTACK_IOS_* env paths. Each test calls makeWorkDir() for its own +// dir instead; afterEach cleans up every dir created during the test. +const createdWorkDirs: string[] = []; +function makeWorkDir(): string { + const dir = mkdtempSync(join(tmpdir(), 'ios-e2e-')); + createdWorkDirs.push(dir); + return dir; +} -beforeEach(() => { - workDir = mkdtempSync(join(tmpdir(), 'ios-e2e-')); -}); - -afterEach(() => { - rmSync(workDir, { recursive: true, force: true }); +// Clean up ONCE after all tests, not per-test. Under `bun test --concurrent` +// an afterEach that drains the shared array would delete still-running tests' +// workDirs the moment ANY test finishes, vanishing their audit/attempts files +// mid-assertion. afterAll runs after every concurrent test has settled. +afterAll(() => { + for (const dir of createdWorkDirs) { + rmSync(dir, { recursive: true, force: true }); + } + createdWorkDirs.length = 0; }); interface StubState { @@ -152,6 +165,7 @@ async function fetchJson(method: string, url: string, init: { headers?: Record { test('NO_DEVICE: codegen runs against a SwiftUI fixture and emits valid accessors', () => { + const workDir = makeWorkDir(); const srcDir = join(workDir, 'app-src'); mkdirSync(srcDir); writeFileSync(join(srcDir, 'AppState.swift'), ` @@ -183,6 +197,7 @@ class AppState { }); test('NO_DEVICE: cache hit on rerun', () => { + const workDir = makeWorkDir(); const srcDir = join(workDir, 'app-src'); mkdirSync(srcDir); writeFileSync(join(srcDir, 'AppState.swift'), '@Observable class A { @Snapshotable var x: Int = 0 }'); @@ -194,6 +209,7 @@ class AppState { }); test('NO_DEVICE: schema mismatch returns 409 on restore', async () => { + const workDir = makeWorkDir(); const stub = await startStubStateServer({ loggedIn: false, username: '', rawTaps: [] }); try { const tunnel: DeviceTunnel = { @@ -237,6 +253,7 @@ class AppState { describe('ios-qa E2E (agent-flow simulation)', () => { test('SCENARIO: acquire → snapshot → restore → tap → release', async () => { + const workDir = makeWorkDir(); const initial: StubState = { loggedIn: false, username: '', rawTaps: [] }; const stub = await startStubStateServer(initial); try { @@ -303,6 +320,7 @@ describe('ios-qa E2E (agent-flow simulation)', () => { }); test('SCENARIO: contention — second session-acquire returns 423 while first holds', async () => { + const workDir = makeWorkDir(); const stub = await startStubStateServer({ loggedIn: false, username: '', rawTaps: [] }); try { const tunnel: DeviceTunnel = { @@ -333,14 +351,16 @@ describe('ios-qa E2E (agent-flow simulation)', () => { }); test('SCENARIO: tailnet allowlist gate + mint + audit log', async () => { + const workDir = makeWorkDir(); const stub = await startStubStateServer({ loggedIn: false, username: '', rawTaps: [] }); try { const allowPath = join(workDir, 'allowlist.json'); const auditPath = join(workDir, 'audit.jsonl'); const attemptsPath = join(workDir, 'attempts.jsonl'); - process.env.GSTACK_IOS_ALLOWLIST_PATH = allowPath; - process.env.GSTACK_IOS_AUDIT_PATH = auditPath; - process.env.GSTACK_IOS_ATTEMPTS_PATH = attemptsPath; + // Pass paths as daemon OPTIONS, not process.env — env is process-global + // and races across concurrent tests (the cause of the original + // intermittent failures). GSTACK_IOS_TAILNET_BIND is read from env but + // is the same constant for every tailnet test, so it can't diverge. process.env.GSTACK_IOS_TAILNET_BIND = '127.0.0.1'; const tunnel: DeviceTunnel = { @@ -352,6 +372,9 @@ describe('ios-qa E2E (agent-flow simulation)', () => { const daemon = await startDaemon({ loopbackPort: 0, tailnetEnabled: true, + allowlistPath: allowPath, + auditPath, + attemptsPath, pidfilePath: join(workDir, 'daemon.pid'), tunnelProvider: async () => tunnel, probeImpl: async () => ({ ok: true, ownIdentity: 'mac@e2e' }), @@ -406,9 +429,6 @@ describe('ios-qa E2E (agent-flow simulation)', () => { expect(attempts).toMatch(/"reason":"identity_not_allowed"/); } finally { await daemon.close(); - delete process.env.GSTACK_IOS_ALLOWLIST_PATH; - delete process.env.GSTACK_IOS_AUDIT_PATH; - delete process.env.GSTACK_IOS_ATTEMPTS_PATH; delete process.env.GSTACK_IOS_TAILNET_BIND; } } finally { @@ -417,19 +437,20 @@ describe('ios-qa E2E (agent-flow simulation)', () => { }); test('SCENARIO: capability-tier enforcement — observe token cannot /tap', async () => { + const workDir = makeWorkDir(); const stub = await startStubStateServer({ loggedIn: false, username: '', rawTaps: [] }); try { const allowPath = join(workDir, 'allowlist.json'); - process.env.GSTACK_IOS_ALLOWLIST_PATH = allowPath; - process.env.GSTACK_IOS_AUDIT_PATH = join(workDir, 'audit.jsonl'); - process.env.GSTACK_IOS_ATTEMPTS_PATH = join(workDir, 'attempts.jsonl'); - + // Paths via daemon options, not process.env (concurrency-safe). const tunnel: DeviceTunnel = { udid: 'CAP-UDID', ipv6Addr: '127.0.0.1', port: stub.port, bootTokenRotated: DEVICE_TOKEN, }; const daemon = await startDaemon({ loopbackPort: 0, tailnetEnabled: true, + allowlistPath: allowPath, + auditPath: join(workDir, 'audit.jsonl'), + attemptsPath: join(workDir, 'attempts.jsonl'), pidfilePath: join(workDir, 'daemon.pid'), tunnelProvider: async () => tunnel, probeImpl: async () => ({ ok: true, ownIdentity: 'mac@e2e' }), @@ -463,9 +484,6 @@ describe('ios-qa E2E (agent-flow simulation)', () => { expect((tap.body as { error: string }).error).toBe('capability_insufficient'); } finally { await daemon.close(); - delete process.env.GSTACK_IOS_ALLOWLIST_PATH; - delete process.env.GSTACK_IOS_AUDIT_PATH; - delete process.env.GSTACK_IOS_ATTEMPTS_PATH; } } finally { stub.server.close(); @@ -477,6 +495,7 @@ describe('ios-qa E2E (agent-flow simulation)', () => { (HAS_DEVICE ? describe : describe.skip)('ios-qa E2E (with device)', () => { test('WITH_DEVICE: full agent loop against a real iPhone', () => { + const workDir = makeWorkDir(); // Stub — real implementation requires `devicectl` + an attached iPhone. // Documented in ios-qa/SKILL.md.tmpl under "Manual smoke test". expect(HAS_DEVICE).toBe(true);