From 7c25b351f7ffb8ab63ae79d3ea55b02ecfcd6dfa Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 09:25:32 -0700 Subject: [PATCH] test(comfyui): refuse a real-host port another run holds (#310) The real-host config waited on the /system_stats URL, which any server on the port answers. When two runs started on 8189 within a few seconds of each other, the second run's ComfyUI failed to bind ("Port 8189 is already in use"), but Playwright had already seen the first run's host answer and ran the second run's tests against it. When the first run finished and stopped its host, the second run failed mid-test with ECONNREFUSED, ECONNRESET or ERR_CONNECTION_REFUSED, and passed when rerun alone. The launcher now refuses a port that is in use, and reports ready only when /system_stats names its own base directory. The config waits for that line instead of the URL, and every test checks again that the host answering the port is the one its run started. --- TESTING.md | 7 ++++ tests/comfyui/host-fixture.mjs | 11 ++++- tests/vendor/comfyui-config.test.mjs | 14 +++++++ tests/vendor/comfyui-host.test.mjs | 51 ++++++++++++++++++++++- tools/vendor/comfyui-host.mjs | 61 ++++++++++++++++++++++++++-- tools/vendor/comfyui.config.mjs | 7 +++- 6 files changed, 145 insertions(+), 6 deletions(-) diff --git a/TESTING.md b/TESTING.md index 16cf4cc..2b29c97 100644 --- a/TESTING.md +++ b/TESTING.md @@ -121,6 +121,13 @@ npm run --prefix tools/vendor test:comfyui CI performs this setup automatically. `PM_COMFY_URL` can select another `http://127.0.0.1:PORT/` address (default port 8189). This is an owned managed server: a running host is never reused. Do not point it at your live checkout. +One port serves one run. The launcher refuses a port that is already in use and +reports ready only when `/system_stats` names its own base directory, so two runs +started together on 8189 fail at startup ("already in use" or "answered by a +ComfyUI host this run did not start") instead of one run testing the other's +host and losing it mid-test (`ECONNREFUSED`, `ERR_CONNECTION_REFUSED`, Network +Error). Every test checks the answering host again. Agents sharing a machine +give each run its own `PM_COMFY_URL` port. The launcher verifies the upstream SHA and clean tracked files, snapshots only tracked PromptManager files, and creates temporary user/input/output/database/ configuration/log directories. Personal untracked files are excluded. It runs diff --git a/tests/comfyui/host-fixture.mjs b/tests/comfyui/host-fixture.mjs index 8246ad2..a88cb77 100644 --- a/tests/comfyui/host-fixture.mjs +++ b/tests/comfyui/host-fixture.mjs @@ -1,11 +1,20 @@ import { expect } from '../../tools/vendor/node_modules/@playwright/test/index.mjs'; import runtime from '../../tools/vendor/comfyui-runtime.json' with { type: 'json' }; +import { hostBaseDirectory } from '../../tools/vendor/comfyui-host.mjs'; export function installHostHooks(test, expectedTypes = ['PromptManager', 'PromptManagerText', 'PromptSearchList']) { test.beforeEach(async ({ page, context, request }, testInfo) => { const response = await request.get('/system_stats'); expect(response.ok()).toBe(true); - const info = (await response.json()).system; + const stats = await response.json(); + // comfyui.config.mjs's launcher reports the host it owns. The execution suite has no + // web server: it starts its own host per worker behind a port refusal. + if (testInfo.config.webServer) { + const ownedBase = process.env.PM_COMFY_HOST_BASE; + expect(ownedBase, 'the launcher reported the host it started').toBeTruthy(); + expect(hostBaseDirectory(stats), 'the host answering the port is the one this run started').toBe(ownedBase); + } + const info = stats.system; expect(info.comfyui_version).toBe(runtime.serverVersion); expect(info.pytorch_version).toBe(`${runtime.torchVersion}+cpu`); expect(info.required_frontend_version).toBe(runtime.frontendVersion); diff --git a/tests/vendor/comfyui-config.test.mjs b/tests/vendor/comfyui-config.test.mjs index 971dee7..e13e505 100644 --- a/tests/vendor/comfyui-config.test.mjs +++ b/tests/vendor/comfyui-config.test.mjs @@ -51,3 +51,17 @@ test('CI retains stock, execution and pack suites in order with uploaded executi assert.deepEqual(commands, ['test:comfyui', 'test:executions', 'test:compat']); assert.match(workflow, /path: tools\/vendor\/test-results\//); }); + +test('real-host config waits for its own launcher, never for whichever server holds the port', async t => { + const previous = process.env.PM_COMFY_ROOT; + process.env.PM_COMFY_ROOT = '/unused-pinned-checkout'; + t.after(() => { if (previous === undefined) delete process.env.PM_COMFY_ROOT; else process.env.PM_COMFY_ROOT = previous; }); + const { default: config } = await import('../../tools/vendor/comfyui.config.mjs'); + const [server] = config.webServer; + assert.equal(server.url, undefined); + assert.equal(server.port, undefined); + assert.equal(server.reuseExistingServer, false); + const ready = server.wait.stdout.exec('Starting server\nPM_COMFY_HOST_READY base-directory=/tmp/comfyassets-host-a b\r\nlater'); + assert.equal(ready.groups.pm_comfy_host_base, '/tmp/comfyassets-host-a b'); + assert.equal(server.wait.stdout.exec('To see the GUI go to: http://127.0.0.1:8189'), null); +}); diff --git a/tests/vendor/comfyui-host.test.mjs b/tests/vendor/comfyui-host.test.mjs index a39a289..010f069 100644 --- a/tests/vendor/comfyui-host.test.mjs +++ b/tests/vendor/comfyui-host.test.mjs @@ -6,7 +6,8 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { once } from 'node:events'; import { createServer } from 'node:net'; -import { launchComfyHost } from '../../tools/vendor/comfyui-host.mjs'; +import { createServer as createHttpServer } from 'node:http'; +import { launchComfyHost, assertPortFree, waitForOwnHost, hostBaseDirectory } from '../../tools/vendor/comfyui-host.mjs'; async function fixture(t, program) { const directory = await mkdtemp(join(tmpdir(), 'pm-host-test-')); @@ -232,3 +233,51 @@ test('execution launcher refuses an occupied port without touching an existing s await new Promise(resolve => server.close(resolve)); } }); + +// A stand-in ComfyUI that answers /system_stats with its own argv, as the real host does. +const answersOwnArgv = `const args = process.argv.slice(1); +require('node:http').createServer((request, response) => { + response.setHeader('content-type', 'application/json'); + response.end(JSON.stringify({ system: { argv: args } })); +}).listen(Number(args[args.indexOf('--port') + 1]), '127.0.0.1');`; + +async function listen(t, port, handler = (request, response) => response.end()) { + const server = createHttpServer(handler); + await new Promise((resolve, reject) => { server.once('error', reject); server.listen(port, '127.0.0.1', resolve); }); + t.after(() => new Promise(resolve => server.close(resolve))); + return server; +} + +test('real-host launcher refuses a port another run already holds and leaves that server running', async t => { + const server = await listen(t, 8199); + await assert.rejects(assertPortFree(8199), /8199 is already in use.*never shared/s); + assert.equal(server.listening, true); +}); + +test('real-host launcher is ready only when its own ComfyUI answers the port', async t => { + const options = await fixture(t, answersOwnArgv); + await assertPortFree(options.port); + const host = await launchComfyHost(options); + t.after(() => host.stop()); + await waitForOwnHost(host, options.port); + const stats = await (await fetch(`http://127.0.0.1:${options.port}/system_stats`)).json(); + assert.equal(hostBaseDirectory(stats), host.stateDirectory); +}); + +test('real-host launcher refuses a host it did not start that answers before its own binds', async t => { + await listen(t, 8199, (request, response) => { + response.setHeader('content-type', 'application/json'); + response.end(JSON.stringify({ system: { argv: ['main.py', '--base-directory', '/other-run-state'] } })); + }); + const options = await fixture(t, idle); + const host = await launchComfyHost(options); + t.after(() => host.stop()); + await assert.rejects(waitForOwnHost(host, options.port), /did not start \(base directory \/other-run-state\)/); +}); + +test('real-host launcher reports a host that exits before it answers', async t => { + const options = await fixture(t, 'process.exit(3);'); + const host = await launchComfyHost(options); + t.after(() => host.stop()); + await assert.rejects(waitForOwnHost(host, options.port), /exited before it answered on port 8199 \(exit=3/); +}); diff --git a/tools/vendor/comfyui-host.mjs b/tools/vendor/comfyui-host.mjs index a545487..49f264e 100644 --- a/tools/vendor/comfyui-host.mjs +++ b/tools/vendor/comfyui-host.mjs @@ -1,4 +1,5 @@ import { writeFile } from 'node:fs/promises'; +import { createServer } from 'node:net'; import { join, resolve } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { launchComfyHost as launchSharedHost } from '@comfyassets/toolkit/testing/host'; @@ -7,6 +8,9 @@ import runtime from './comfyui-runtime.json' with { type: 'json' }; export { launchSharedHost }; const repositoryRoot = fileURLToPath(new URL('../../', import.meta.url)); +// comfyui.config.mjs waits for this line; its named group becomes PM_COMFY_HOST_BASE in the test workers. +export const HOST_READY_MARKER = 'PM_COMFY_HOST_READY'; +const READY_POLL_MS = 250; export async function preparePromptManagerPlugin({ installed, stateDirectory }) { const configPath = join(installed, 'config.json'); @@ -27,17 +31,68 @@ export function launchComfyHost({ checkout, repository = repositoryRoot, }); } +export async function assertPortFree(port) { + await new Promise((resolve, reject) => { + const probe = createServer(); + probe.once('error', error => reject(new Error(`ComfyUI host port ${port} is already in use, often by another ` + + 'real-host run. A managed host is never shared: stop that run or set PM_COMFY_URL to a free ' + + 'http://127.0.0.1:PORT/ address.', { cause: error }))); + probe.listen({ host: '127.0.0.1', port, exclusive: true }, () => probe.close(error => error ? reject(error) : resolve())); + }); +} + +export function hostBaseDirectory(stats) { + const argv = stats?.system?.argv; + if (!Array.isArray(argv)) return null; + const index = argv.indexOf('--base-directory'); + return index >= 0 ? argv[index + 1] ?? null : null; +} + +// Ready means this launcher's own ComfyUI answers: /system_stats reports its base directory. +// Any other answer on the port comes from a host this run did not start, so the run must not use it. +export async function waitForOwnHost(host, port, { fetchImpl = fetch } = {}) { + let exited = null; + host.finished.then(result => { exited = result; }); + const url = `http://127.0.0.1:${port}/system_stats`; + for (;;) { + let stats = null; + try { stats = await (await fetchImpl(url)).json(); } + catch { stats = null; } + if (stats) { + const base = hostBaseDirectory(stats); + if (base === host.stateDirectory) return; + throw new Error(`Port ${port} is answered by a ComfyUI host this run did not start ` + + `(base directory ${base ?? 'unknown'}); another run is using the port.`); + } + if (exited) { + throw new Error(`ComfyUI host exited before it answered on port ${port} ` + + `(exit=${exited.code}, signal=${exited.signal || 'none'}).`); + } + await new Promise(resolve => setTimeout(resolve, READY_POLL_MS)); + } +} + if (process.argv[1] && import.meta.url === pathToFileURL(resolve(process.argv[1])).href) { try { - const host = await launchComfyHost({ checkout: process.env.PM_COMFY_ROOT, - port: Number(process.env.PM_COMFY_PORT || 8189) }); + const port = Number(process.env.PM_COMFY_PORT || 8189); + await assertPortFree(port); + const host = await launchComfyHost({ checkout: process.env.PM_COMFY_ROOT, port }); const stop = () => { host.stop().catch(error => { console.error(error.message); process.exitCode = 1; }); }; process.once('SIGINT', stop); process.once('SIGTERM', stop); + let refused = false; + try { + await waitForOwnHost(host, port); + console.log(`${HOST_READY_MARKER} base-directory=${host.stateDirectory}`); + } catch (error) { + refused = true; + console.error(error.message); + await host.stop(); + } const result = await host.finished; process.removeListener('SIGINT', stop); process.removeListener('SIGTERM', stop); if (result.code) console.error(`ComfyUI host exited with status ${result.code}; see tools/vendor/test-results/comfyui-host.log`); - process.exitCode = result.code; + process.exitCode = refused ? 1 : result.code; } catch (error) { console.error(error.message); process.exitCode = 1; } } diff --git a/tools/vendor/comfyui.config.mjs b/tools/vendor/comfyui.config.mjs index 6d7dc76..0c121a4 100644 --- a/tools/vendor/comfyui.config.mjs +++ b/tools/vendor/comfyui.config.mjs @@ -1,5 +1,6 @@ import { defineConfig } from './node_modules/@playwright/test/index.mjs'; import { fileURLToPath } from 'node:url'; +import { HOST_READY_MARKER } from './comfyui-host.mjs'; if (!process.env.PM_COMFY_ROOT) throw new Error('Set PM_COMFY_ROOT to a pinned disposable ComfyUI checkout'); const host = new URL(process.env.PM_COMFY_URL || 'http://127.0.0.1:8189/'); @@ -7,7 +8,11 @@ if (host.protocol !== 'http:' || host.hostname !== '127.0.0.1' || !host.port) th const servers = [{ command: 'node tools/vendor/comfyui-host.mjs', cwd: fileURLToPath(new URL('../../', import.meta.url)), - url: new URL('system_stats', host).href, timeout: 120000, reuseExistingServer: false, + // No URL wait: any server on the port would satisfy it, including another run's host that + // then stops mid-test. The launcher refuses a used port and prints this line only once its + // own ComfyUI answers; the group reaches the tests as PM_COMFY_HOST_BASE. + wait: { stdout: new RegExp(`^${HOST_READY_MARKER} base-directory=(?.+?)\r?$`, 'm') }, + timeout: 120000, reuseExistingServer: false, env: { PM_COMFY_PORT: host.port }, gracefulShutdown: { signal: 'SIGTERM', timeout: 10000 }, }];