From e118531be4b7ae363f734993b0c3dd2fc9aea172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 14 Aug 2026 14:14:56 +0200 Subject: [PATCH] Enforce E2E process and port ownership Assisted-By: devx/215513a9-13fa-4e74-bb8f-79faee6e4f09 --- packages/e2e/package.json | 1 + packages/e2e/playwright.config.ts | 2 + packages/e2e/scripts/prime-browser-auth.ts | 14 +- packages/e2e/setup/app.ts | 2 +- packages/e2e/setup/cli.ts | 88 ++++++----- packages/e2e/setup/env.ts | 4 + packages/e2e/setup/global-auth.ts | 14 +- packages/e2e/setup/ports.ts | 35 +++++ packages/e2e/setup/process.ts | 120 +++++++++++++++ packages/e2e/setup/store.ts | 11 +- packages/e2e/tests/app-dev-server.spec.ts | 24 +-- packages/e2e/tests/dev-hot-reload.spec.ts | 6 +- packages/e2e/tests/multi-config-dev.spec.ts | 4 +- packages/e2e/tests/ownership.spec.ts | 155 ++++++++++++++++++++ packages/e2e/tests/toml-config.spec.ts | 2 +- pnpm-lock.yaml | 3 + 16 files changed, 421 insertions(+), 64 deletions(-) create mode 100644 packages/e2e/setup/ports.ts create mode 100644 packages/e2e/setup/process.ts create mode 100644 packages/e2e/tests/ownership.spec.ts diff --git a/packages/e2e/package.json b/packages/e2e/package.json index 119246f80ee..e4cc00b5c28 100644 --- a/packages/e2e/package.json +++ b/packages/e2e/package.json @@ -31,6 +31,7 @@ "devDependencies": { "@iarna/toml": "2.2.5", "@playwright/test": "^1.61.1", + "@shopify/cli-kit": "4.6.0", "@shopify/toml-patch": "0.3.0", "@types/node": "22.20.1", "dotenv": "16.6.1", diff --git a/packages/e2e/playwright.config.ts b/packages/e2e/playwright.config.ts index c4e6e148c8f..d156759c02a 100644 --- a/packages/e2e/playwright.config.ts +++ b/packages/e2e/playwright.config.ts @@ -31,6 +31,7 @@ export default defineConfig({ 'tests/smoke-pty.spec.ts', 'tests/fixture-toml.spec.ts', 'tests/auth-diagnostics.spec.ts', + 'tests/ownership.spec.ts', ], }, { @@ -45,6 +46,7 @@ export default defineConfig({ 'tests/smoke-pty.spec.ts', 'tests/fixture-toml.spec.ts', 'tests/auth-diagnostics.spec.ts', + 'tests/ownership.spec.ts', ], dependencies: ['remote-auth'], }, diff --git a/packages/e2e/scripts/prime-browser-auth.ts b/packages/e2e/scripts/prime-browser-auth.ts index 6e6ae2cc6cd..e891d5ff6d1 100644 --- a/packages/e2e/scripts/prime-browser-auth.ts +++ b/packages/e2e/scripts/prime-browser-auth.ts @@ -17,6 +17,7 @@ import {chromium} from '@playwright/test' import {BROWSER_TIMEOUT, CLI_TIMEOUT} from '../setup/constants.js' import {executables} from '../setup/env.js' import {isVisibleWithin} from '../setup/browser.js' +import {observePtyExit, terminateProcessTree} from '../setup/process.js' import {completeLogin} from '../helpers/browser-login.js' import {addLoadtestHeader} from '../helpers/loadtest-header.js' import {stripAnsi} from '../helpers/strip-ansi.js' @@ -160,6 +161,7 @@ async function primeCliAuth(page: Page, email: string, password: string, env: No rows: 30, env: spawnEnv, }) + const exitObserver = observePtyExit(ptyProcess) let output = '' ptyProcess.onData((data: string) => { @@ -178,11 +180,13 @@ async function primeCliAuth(page: Page, email: string, password: string, env: No await completeLogin(page, urlMatch[0], email, password) await waitForText(() => output, 'Logged in', BROWSER_TIMEOUT.max) } finally { - try { - ptyProcess.kill() - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (_error) { - // Process may already be dead. + if (!exitObserver.hasExited()) { + await terminateProcessTree({ + pid: ptyProcess.pid, + command: `node ${executables.cli} auth login`, + owner: 'prime-browser-auth', + waitForExit: exitObserver.waitForExit, + }) } } } diff --git a/packages/e2e/setup/app.ts b/packages/e2e/setup/app.ts index 1f8a3b94fb4..d4504f70e1f 100644 --- a/packages/e2e/setup/app.ts +++ b/packages/e2e/setup/app.ts @@ -315,7 +315,7 @@ export async function configLink( const exitCode = await proc.waitForExit(CLI_TIMEOUT.long) return {exitCode, stdout: proc.getOutput(), stderr: ''} } finally { - proc.kill() + await proc.terminate() } } diff --git a/packages/e2e/setup/cli.ts b/packages/e2e/setup/cli.ts index ad1793d171f..cf7e6e1ea71 100644 --- a/packages/e2e/setup/cli.ts +++ b/packages/e2e/setup/cli.ts @@ -1,5 +1,7 @@ import {CLI_TIMEOUT} from './constants.js' import {createLogger, envFixture, executables} from './env.js' +import {assertPortsAvailable} from './ports.js' +import {observePtyExit, terminateProcessTree} from './process.js' import {stripAnsi} from '../helpers/strip-ansi.js' import {execa, type Options as ExecaOptions} from 'execa' import type {E2EEnv} from './env.js' @@ -30,8 +32,8 @@ export interface SpawnedProcess { sendLine(line: string): void /** Wait for the process to exit */ waitForExit(timeoutMs?: number): Promise - /** Kill the process */ - kill(): void + /** Terminate the complete process tree and wait for it to exit */ + terminate(timeoutMs?: number): Promise /** Get all output captured so far (ANSI stripped) */ getOutput(): string /** The underlying node-pty process */ @@ -49,7 +51,7 @@ export interface CLIProcess { /** * Test-scoped fixture providing CLI process management. - * Tracks all spawned processes and kills them in teardown. + * Tracks all spawned processes and terminates them in teardown. */ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({ cli: async ({env}, use) => { @@ -124,6 +126,7 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({ cliLog.log(env, `spawn: node ${executables.cli}`) cliLog.log(env, args.join(' ')) + const command = ['node', executables.cli, ...args].map((value) => JSON.stringify(value)).join(' ') const ptyProcess = nodePty.spawn('node', [executables.cli, ...args], { name: 'xterm-color', @@ -153,14 +156,9 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({ } }) - let exitCode: number | undefined - let exitResolve: ((code: number) => void) | undefined + const exitObserver = observePtyExit(ptyProcess) ptyProcess.onExit(({exitCode: code}) => { - exitCode = code - if (exitResolve) { - exitResolve(code) - } // Reject any remaining output waiters. reject() removes each waiter // from outputWaiters, so iterate over a snapshot to avoid skipping. for (const waiter of [...outputWaiters]) { @@ -241,29 +239,21 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({ }, waitForExit(timeoutMs = CLI_TIMEOUT.short) { - if (exitCode !== undefined) { - return Promise.resolve(exitCode) - } - - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - reject(new Error(`Timed out after ${timeoutMs}ms waiting for process exit`)) - }, timeoutMs) - - exitResolve = (code) => { - clearTimeout(timer) - resolve(code) - } - }) + return exitObserver.waitForExit(timeoutMs) }, - kill() { - try { - ptyProcess.kill() - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (_error) { - // Process may already be dead - } + async terminate(timeoutMs) { + if (exitObserver.hasExited()) return + + await terminateProcessTree( + { + pid: ptyProcess.pid, + command, + owner: `worker=${env.workerIndex}`, + waitForExit: exitObserver.waitForExit, + }, + timeoutMs, + ) }, getOutput() { @@ -276,11 +266,43 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({ }, } - await use(cli) + let testFailed = false + let testFailure: unknown + try { + await use(cli) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + testFailed = true + testFailure = error + } + + const cleanupFailures: Error[] = [] + const ownedPorts = env.ownedPorts - // Teardown: kill all spawned processes for (const proc of spawnedProcesses) { - proc.kill() + try { + // eslint-disable-next-line no-await-in-loop + await proc.terminate() + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + cleanupFailures.push(error instanceof Error ? error : new Error(String(error))) + } + } + + try { + await assertPortsAvailable(ownedPorts, `worker=${env.workerIndex} phase=release`) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + cleanupFailures.push(error instanceof Error ? error : new Error(String(error))) + } + ownedPorts.splice(0, ownedPorts.length) + + if (testFailed && cleanupFailures.length > 0) { + throw new AggregateError([testFailure, ...cleanupFailures], `[e2e][w${env.workerIndex}] test and cleanup failed`) + } + if (testFailed) throw testFailure + if (cleanupFailures.length > 0) { + throw new AggregateError(cleanupFailures, `[e2e][w${env.workerIndex}] cleanup failed`) } }, }) diff --git a/packages/e2e/setup/env.ts b/packages/e2e/setup/env.ts index 37858cf50e2..f111eb9c589 100644 --- a/packages/e2e/setup/env.ts +++ b/packages/e2e/setup/env.ts @@ -3,6 +3,7 @@ import {test as base} from '@playwright/test' import * as path from 'path' import * as fs from 'fs' import {fileURLToPath} from 'url' +import type {OwnedPort} from './ports.js' const __filename = fileURLToPath(import.meta.url) const __dirname = path.dirname(__filename) @@ -18,6 +19,8 @@ export interface E2EEnv { tempDir: string /** Playwright worker index (0-based) for debug logging */ workerIndex: number + /** Ports claimed by the current test and verified during CLI fixture cleanup */ + ownedPorts: OwnedPort[] } /** Worker context for logging */ @@ -196,6 +199,7 @@ export const envFixture = base.extend<{testSection: void}, {env: E2EEnv}>({ processEnv, tempDir, workerIndex: workerInfo.parallelIndex, + ownedPorts: [], } await use(env) diff --git a/packages/e2e/setup/global-auth.ts b/packages/e2e/setup/global-auth.ts index 0ac051d63d1..364912abcdf 100644 --- a/packages/e2e/setup/global-auth.ts +++ b/packages/e2e/setup/global-auth.ts @@ -8,6 +8,7 @@ import {isVisibleWithin} from './browser.js' import {executables, globalLog} from './env.js' import {authStatePaths} from './auth-state.js' +import {observePtyExit, terminateProcessTree} from './process.js' import { AuthSetupError, isExpectedAuthDestination, @@ -130,6 +131,7 @@ async function authenticateOnce({ } catch (_error) { throw new AuthSetupError('pty-startup', 'spawn-failed') } + const exitObserver = observePtyExit(ptyProcess) let output = '' ptyProcess.onData((data: string) => { @@ -177,11 +179,13 @@ async function authenticateOnce({ throw error } } finally { - try { - ptyProcess.kill() - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (_error) { - // Process may already be dead + if (!exitObserver.hasExited()) { + await terminateProcessTree({ + pid: ptyProcess.pid, + command: `node ${executables.cli} auth login`, + owner: 'global-auth', + waitForExit: exitObserver.waitForExit, + }) } } } diff --git a/packages/e2e/setup/ports.ts b/packages/e2e/setup/ports.ts new file mode 100644 index 00000000000..4723649080a --- /dev/null +++ b/packages/e2e/setup/ports.ts @@ -0,0 +1,35 @@ +import {createServer} from 'node:net' + +export interface OwnedPort { + environmentVariable: string + port: number +} + +export function workerPorts(workerIndex: number): OwnedPort[] { + const portBase = 3457 + workerIndex * 10 + return [ + {environmentVariable: 'SHOPIFY_FLAG_GRAPHIQL_PORT', port: portBase}, + {environmentVariable: 'SHOPIFY_FLAG_THEME_APP_EXTENSION_PORT', port: portBase + 2}, + ] +} + +export async function assertPortsAvailable(ports: OwnedPort[], owner: string): Promise { + const availability = await Promise.all( + ports.map(async (port) => ({...port, available: await isPortAvailable(port.port)})), + ) + const occupiedPorts = availability.filter(({available}) => !available) + + if (occupiedPorts.length > 0) { + const details = occupiedPorts.map(({environmentVariable, port}) => `${environmentVariable}=${port}`).join(', ') + throw new Error(`[e2e][ports] owner=${owner} unavailable=${details}`) + } +} + +function isPortAvailable(port: number): Promise { + return new Promise((resolve) => { + const server = createServer() + server.unref() + server.once('error', () => resolve(false)) + server.listen(port, 'localhost', () => server.close(() => resolve(true))) + }) +} diff --git a/packages/e2e/setup/process.ts b/packages/e2e/setup/process.ts new file mode 100644 index 00000000000..bceb3fce267 --- /dev/null +++ b/packages/e2e/setup/process.ts @@ -0,0 +1,120 @@ +import type {IPty} from 'node-pty' + +interface ProcessTreeOwner { + pid: number + command: string + owner: string + waitForExit: (timeoutMs: number) => Promise +} + +export interface ProcessExitObserver { + hasExited: () => boolean + waitForExit: (timeoutMs: number) => Promise +} + +export function observePtyExit(ptyProcess: Pick): ProcessExitObserver { + let exitCode: number | undefined + const waiters = new Set<(exitCode: number) => void>() + + ptyProcess.onExit(({exitCode: code}) => { + exitCode = code + for (const resolve of waiters) resolve(code) + waiters.clear() + }) + + return { + hasExited: () => exitCode !== undefined, + waitForExit: (timeoutMs) => { + if (exitCode !== undefined) return Promise.resolve(exitCode) + + return new Promise((resolve, reject) => { + const handleExit = (code: number) => { + clearTimeout(timer) + waiters.delete(handleExit) + resolve(code) + } + const timer = setTimeout(() => { + waiters.delete(handleExit) + reject(new Error(`Timed out after ${timeoutMs}ms waiting for process exit`)) + }, timeoutMs) + waiters.add(handleExit) + }) + }, + } +} + +export async function terminateProcessTree(owner: ProcessTreeOwner, timeoutMs = 5_000): Promise { + const signalFailures: string[] = [] + const exitFailures: string[] = [] + + await killTree(owner.pid).catch((error) => signalFailures.push(errorMessage(error))) + killProcessGroup(owner.pid, signalFailures) + await Promise.all([ + owner.waitForExit(timeoutMs).catch((error) => exitFailures.push(errorMessage(error))), + waitForProcessGroupExit(owner.pid, timeoutMs).catch((error) => exitFailures.push(errorMessage(error))), + ]) + + if (exitFailures.length > 0) { + throw new Error( + `[e2e][process] owner=${owner.owner} failed to terminate pid=${owner.pid} command=${JSON.stringify( + owner.command, + )} failures=${JSON.stringify([...signalFailures, ...exitFailures])}`, + ) + } +} + +async function killTree(pid: number): Promise { + // A static import makes Playwright load CLI Kit's UI graph during test discovery. + const {treeKill} = await import('@shopify/cli-kit/node/tree-kill') + + return new Promise((resolve, reject) => { + treeKill(pid, 'SIGKILL', true, (error) => (error ? reject(error) : resolve())) + }) +} + +function killProcessGroup(pid: number, failures: string[]): void { + if (process.platform === 'win32') return + + try { + process.kill(-pid, 'SIGKILL') + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + if (!isMissingProcessError(error)) failures.push(errorMessage(error)) + } +} + +function waitForProcessGroupExit(pid: number, timeoutMs: number): Promise { + if (process.platform === 'win32' || !isProcessGroupRunning(pid)) return Promise.resolve() + + return new Promise((resolve, reject) => { + const interval = setInterval(() => { + if (!isProcessGroupRunning(pid)) { + clearInterval(interval) + clearTimeout(timeout) + resolve() + } + }, 50) + const timeout = setTimeout(() => { + clearInterval(interval) + reject(new Error(`Timed out after ${timeoutMs}ms waiting for process group ${pid} to exit`)) + }, timeoutMs) + }) +} + +function isProcessGroupRunning(pid: number): boolean { + try { + process.kill(-pid, 0) + return true + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + return !isMissingProcessError(error) + } +} + +function isMissingProcessError(error: unknown): boolean { + return error instanceof Error && 'code' in error && error.code === 'ESRCH' +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} diff --git a/packages/e2e/setup/store.ts b/packages/e2e/setup/store.ts index 135aa7e228b..6408ce17826 100644 --- a/packages/e2e/setup/store.ts +++ b/packages/e2e/setup/store.ts @@ -3,6 +3,7 @@ import {appTestFixture} from './app.js' import {isVisibleWithin} from './browser.js' import {BROWSER_TIMEOUT, CLI_TIMEOUT} from './constants.js' import {createLogger, e2eRunSegment, e2eSection, requireEnv} from './env.js' +import {assertPortsAvailable, workerPorts} from './ports.js' import type {CLIProcess, ExecResult} from './cli.js' import type {Locator, Page} from '@playwright/test' @@ -279,10 +280,12 @@ export const storeTestFixture = appTestFixture.extend<{storeFqdn: string}>({ requireEnv(env, 'orgId') const wi = env.workerIndex - // Unique ports per worker to avoid EADDRINUSE when running in parallel - const portBase = 3457 + wi * 10 - env.processEnv.SHOPIFY_FLAG_GRAPHIQL_PORT = String(portBase) - env.processEnv.SHOPIFY_FLAG_THEME_APP_EXTENSION_PORT = String(portBase + 2) + const ports = workerPorts(wi) + await assertPortsAvailable(ports, `worker=${wi} phase=claim`) + env.ownedPorts.push(...ports) + for (const {environmentVariable, port} of ports) { + env.processEnv[environmentVariable] = String(port) + } const storeName = generateStoreName(wi) const fqdn = await createDevStoreWithCli({cli, workerIndex: wi, storeName, orgId: env.orgId}) diff --git a/packages/e2e/tests/app-dev-server.spec.ts b/packages/e2e/tests/app-dev-server.spec.ts index 19bbf43ec57..4312ef31347 100644 --- a/packages/e2e/tests/app-dev-server.spec.ts +++ b/packages/e2e/tests/app-dev-server.spec.ts @@ -38,19 +38,23 @@ test.describe('App dev server', () => { env: {CI: '', SHOPIFY_FLAG_STORE: storeFqdn}, }) - // Step 3: Wait for the ready message - await dev.waitForOutput('Ready, watching for changes in your app', CLI_TIMEOUT.medium) + try { + // Step 3: Wait for the ready message + await dev.waitForOutput('Ready, watching for changes in your app', CLI_TIMEOUT.medium) - // Step 4: Verify keyboard shortcuts are shown (indicates TTY mode is working) - const output = dev.getOutput() - expect(output).toContain('q') + // Step 4: Verify keyboard shortcuts are shown (indicates TTY mode is working) + const output = dev.getOutput() + expect(output).toContain('q') - // Step 5: Press q to quit - dev.sendKey('q') + // Step 5: Press q to quit + dev.sendKey('q') - // Step 6: Wait for clean exit - const exitCode = await dev.waitForExit(CLI_TIMEOUT.short) - expect(exitCode, `dev exited with non-zero code. Output:\n${dev.getOutput()}`).toBe(0) + // Step 6: Wait for clean exit + const exitCode = await dev.waitForExit(CLI_TIMEOUT.short) + expect(exitCode, `dev exited with non-zero code. Output:\n${dev.getOutput()}`).toBe(0) + } finally { + await dev.terminate() + } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. if (!process.env.E2E_SKIP_TEARDOWN) { diff --git a/packages/e2e/tests/dev-hot-reload.spec.ts b/packages/e2e/tests/dev-hot-reload.spec.ts index f976451aaf5..86c3d763a07 100644 --- a/packages/e2e/tests/dev-hot-reload.spec.ts +++ b/packages/e2e/tests/dev-hot-reload.spec.ts @@ -84,7 +84,7 @@ test.describe('Dev hot reload', () => { console.error(`[hot-reload app-config] Captured PTY output:\n${proc.getOutput()}`) throw error } finally { - proc.kill() + await proc.terminate() } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. @@ -142,7 +142,7 @@ test.describe('Dev hot reload', () => { console.error(`[hot-reload create] Captured PTY output:\n${proc.getOutput()}`) throw error } finally { - proc.kill() + await proc.terminate() } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. @@ -206,7 +206,7 @@ test.describe('Dev hot reload', () => { console.error(`[hot-reload delete] Captured PTY output:\n${proc.getOutput()}`) throw error } finally { - proc.kill() + await proc.terminate() } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. diff --git a/packages/e2e/tests/multi-config-dev.spec.ts b/packages/e2e/tests/multi-config-dev.spec.ts index d2559fc783e..5570141dc83 100644 --- a/packages/e2e/tests/multi-config-dev.spec.ts +++ b/packages/e2e/tests/multi-config-dev.spec.ts @@ -87,7 +87,7 @@ extensions_summary = "E2E staging app extensions" console.error(`[multi-config dev] Captured PTY output:\n${proc.getOutput()}`) throw error } finally { - proc.kill() + await proc.terminate() } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. @@ -168,7 +168,7 @@ extensions_summary = "E2E staging app extensions" console.error(`[multi-config default] Captured PTY output:\n${proc.getOutput()}`) throw error } finally { - proc.kill() + await proc.terminate() } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. diff --git a/packages/e2e/tests/ownership.spec.ts b/packages/e2e/tests/ownership.spec.ts new file mode 100644 index 00000000000..8669f1a6fce --- /dev/null +++ b/packages/e2e/tests/ownership.spec.ts @@ -0,0 +1,155 @@ +/* eslint-disable no-await-in-loop, no-restricted-imports */ +import {assertPortsAvailable, workerPorts} from '../setup/ports.js' +import {terminateProcessTree} from '../setup/process.js' +import {expect, test} from '@playwright/test' +import {execa} from 'execa' +import {createServer} from 'node:net' +import * as fs from 'node:fs' +import * as path from 'node:path' + +test.describe('E2E resource ownership', () => { + // eslint-disable-next-line no-empty-pattern + test('terminates a spawned process and its child', async ({}, testInfo) => { + fs.mkdirSync(testInfo.outputDir, {recursive: true}) + const tempDir = fs.mkdtempSync(path.join(testInfo.outputDir, 'process-')) + const childPidPath = path.join(tempDir, 'child.pid') + const rootProcess = execa( + 'node', + [ + '-e', + ` + const {spawn} = require('node:child_process') + const {writeFileSync} = require('node:fs') + const child = spawn( + process.execPath, + ['-e', "process.on('SIGTERM', () => {}); setInterval(() => {}, 1_000)"], + {stdio: 'ignore'}, + ) + writeFileSync(process.argv[1], String(child.pid)) + process.on('SIGTERM', () => {}) + setInterval(() => {}, 1_000) + `, + childPidPath, + ], + {detached: process.platform !== 'win32', reject: false}, + ) + if (!rootProcess.pid) throw new Error('Test process did not expose its PID') + + const childPid = Number(await waitForFile(childPidPath)) + + try { + await terminateProcessTree( + { + pid: rootProcess.pid, + command: 'ownership test process', + owner: 'test', + waitForExit: async (timeoutMs) => { + const result = await withTimeout(rootProcess, timeoutMs) + return result.exitCode ?? 1 + }, + }, + 2_000, + ) + + await expect(waitForProcessExit(childPid)).resolves.toBeUndefined() + } finally { + terminateExactProcess(childPid) + terminateExactProcess(rootProcess.pid) + fs.rmSync(tempDir, {recursive: true, force: true}) + } + }) + + test('reports the owner and configured variable for an occupied port', async () => { + const server = createServer() + await new Promise((resolve) => server.listen(0, 'localhost', resolve)) + const address = server.address() + if (!address || typeof address === 'string') throw new Error('Test server did not expose its port') + + try { + await expect( + assertPortsAvailable( + [{environmentVariable: 'SHOPIFY_FLAG_GRAPHIQL_PORT', port: address.port}], + 'worker=3 phase=claim', + ), + ).rejects.toThrow( + `[e2e][ports] owner=worker=3 phase=claim unavailable=SHOPIFY_FLAG_GRAPHIQL_PORT=${address.port}`, + ) + } finally { + await new Promise((resolve, reject) => server.close((error) => (error ? reject(error) : resolve()))) + } + }) + + test('reports process ownership when termination fails', async () => { + const missingPid = 99_999_999 + + await expect( + terminateProcessTree({ + pid: missingPid, + command: 'shopify app dev', + owner: 'worker=4', + waitForExit: () => Promise.reject(new Error('process did not exit')), + }), + ).rejects.toThrow(`[e2e][process] owner=worker=4 failed to terminate pid=${missingPid} command="shopify app dev"`) + }) + + test('assigns distinct fixed ports to each worker', () => { + expect(workerPorts(0)).toEqual([ + {environmentVariable: 'SHOPIFY_FLAG_GRAPHIQL_PORT', port: 3457}, + {environmentVariable: 'SHOPIFY_FLAG_THEME_APP_EXTENSION_PORT', port: 3459}, + ]) + expect(workerPorts(1)).toEqual([ + {environmentVariable: 'SHOPIFY_FLAG_GRAPHIQL_PORT', port: 3467}, + {environmentVariable: 'SHOPIFY_FLAG_THEME_APP_EXTENSION_PORT', port: 3469}, + ]) + }) +}) + +async function waitForFile(filePath: string): Promise { + for (let attempt = 0; attempt < 40; attempt++) { + if (fs.existsSync(filePath)) return fs.readFileSync(filePath, 'utf8') + await new Promise((resolve) => setTimeout(resolve, 50)) + } + throw new Error(`Timed out waiting for ${filePath}`) +} + +async function waitForProcessExit(pid: number): Promise { + for (let attempt = 0; attempt < 40; attempt++) { + if (!isProcessRunning(pid)) return + await new Promise((resolve) => setTimeout(resolve, 50)) + } + throw new Error(`Process ${pid} is still running`) +} + +function isProcessRunning(pid: number): boolean { + try { + process.kill(pid, 0) + return true + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + +function terminateExactProcess(pid: number): void { + if (!isProcessRunning(pid)) return + try { + process.kill(pid, 'SIGKILL') + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + // The process exited after the ownership check. + } +} + +async function withTimeout(promise: Promise, timeoutMs: number): Promise { + let timer: NodeJS.Timeout | undefined + try { + return await Promise.race([ + promise, + new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new Error(`Timed out after ${timeoutMs}ms`)), timeoutMs) + }), + ]) + } finally { + if (timer) clearTimeout(timer) + } +} diff --git a/packages/e2e/tests/toml-config.spec.ts b/packages/e2e/tests/toml-config.spec.ts index 3f93816cb95..14e25f1b355 100644 --- a/packages/e2e/tests/toml-config.spec.ts +++ b/packages/e2e/tests/toml-config.spec.ts @@ -80,7 +80,7 @@ test.describe('TOML config regression', () => { console.error(`[toml-config dev] Captured PTY output:\n${proc.getOutput()}`) throw error } finally { - proc.kill() + await proc.terminate() } } finally { // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ac9dc0316e9..c3191246ea0 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -534,6 +534,9 @@ importers: '@playwright/test': specifier: ^1.61.1 version: 1.61.1 + '@shopify/cli-kit': + specifier: 4.6.0 + version: link:../cli-kit '@shopify/toml-patch': specifier: 0.3.0 version: 0.3.0