From a90dd091f044c0f6ce6f1f07c53015b268ed09c3 Mon Sep 17 00:00:00 2001 From: Michal Date: Sat, 8 Aug 2026 18:49:42 +0100 Subject: [PATCH] fix(smoke): actually clean up smoke-test resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 341 of 356 projects on the shared mcpd were smoke-test leftovers, plus 11 never-expiring `smoke-*` mcptokens sitting on the real `mcpctl-development` and `sre` projects. Enough to make the project picker unusable. Two causes, both silent: - `delete project X --force` — `--force` is valid on `create project` but NOT on `delete`, so every cleanup call exited non-zero and deleted nothing. project-llm-ref's afterAll looked correct and had never worked, which is why there were 84 each of smoke-proj-{ok,orphan,none}-*. - mcptoken.smoke had no afterAll at all; its cleanup was an `it()` at the end of the file, so it was skipped whenever an earlier assertion failed. Cleanup now lives in `afterAll` (runs on failure too) and is no longer gated on the health probe: the project is created through mcpd, which can be up when the gateway probe is not, and deleting a project that was never created is a no-op. Adds `pnpm smoke:clean` for the case afterAll cannot cover — a killed process (CI timeout, Ctrl-C, OOM). Dry-run by default, `--yes` to apply. Only touches `smoke-*` names and protects the shared `smoke-data` / `smoke-aws-docs` fixture, which five suites depend on and which must survive a concurrent run. Verified end to end: created a throwaway smoke project, confirmed the dry run lists without deleting, then `--yes` removed it and left all 15 real projects untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017BMXdb2qZbPSh8Q7XpTyjB --- package.json | 3 +- scripts/clean-smoke-resources.ts | 101 ++++++++++++++++++ .../tests/smoke/mcptoken.smoke.test.ts | 22 +++- .../tests/smoke/project-llm-ref.smoke.test.ts | 17 +-- 4 files changed, 130 insertions(+), 13 deletions(-) create mode 100644 scripts/clean-smoke-resources.ts diff --git a/package.json b/package.json index 5658419..de493e0 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,8 @@ "mcpd:deploy": "bash deploy.sh", "mcpd:deploy-dry": "bash deploy.sh --dry-run", "mcpd:logs": "bash logs.sh", - "typecheck:pi-ext": "tsc -p src/pi-ext/tsconfig.json" + "typecheck:pi-ext": "tsc -p src/pi-ext/tsconfig.json", + "smoke:clean": "tsx scripts/clean-smoke-resources.ts" }, "engines": { "node": ">=20.0.0", diff --git a/scripts/clean-smoke-resources.ts b/scripts/clean-smoke-resources.ts new file mode 100644 index 0000000..238ebe3 --- /dev/null +++ b/scripts/clean-smoke-resources.ts @@ -0,0 +1,101 @@ +#!/usr/bin/env node +/** + * Sweep per-run smoke-test resources off the shared mcpd. + * + * The suites clean up in `afterAll`, which covers assertion failures but not a + * killed process (CI timeout, Ctrl-C, OOM). Over time that leaked hundreds of + * `smoke-*` projects and never-expiring `smoke-*` mcptokens onto the shared + * server, to the point where the project picker was unusable. + * + * Run after a smoke run, or any time the server looks cluttered: + * pnpm smoke:clean # show what would go + * pnpm smoke:clean --yes # actually delete + * + * Only names matching SMOKE_PREFIX are ever considered, and the shared fixture + * (`smoke-data` / `smoke-aws-docs`, provisioned from + * tests/smoke/fixtures/smoke-data.yaml and depended on by five suites) is + * protected — deleting it mid-run would break a concurrent suite. + */ +import { execFileSync } from 'node:child_process'; + +const SMOKE_PREFIX = /^smoke-/; + +/** Shared, intentionally long-lived fixture — never swept. */ +const PROTECTED = new Set(['smoke-data', 'smoke-aws-docs']); + +const APPLY = process.argv.includes('--yes') || process.argv.includes('-y'); + +interface Named { id?: string; name?: string; projectName?: string; status?: string } + +function mcpctl(args: string[]): string { + return execFileSync('mcpctl', args, { encoding: 'utf-8', maxBuffer: 32 * 1024 * 1024 }); +} + +function list(resource: string, extra: string[] = []): Named[] { + try { + const out = mcpctl(['get', resource, '-o', 'json', ...extra]); + const parsed: unknown = JSON.parse(out || '[]'); + return Array.isArray(parsed) ? (parsed as Named[]) : []; + } catch { + return []; + } +} + +function sweepable(rows: Named[]): Named[] { + return rows.filter((r) => typeof r.name === 'string' && SMOKE_PREFIX.test(r.name) && !PROTECTED.has(r.name)); +} + +function remove(resource: string, row: Named, extra: string[] = []): boolean { + const name = row.name ?? row.id ?? ''; + if (!SMOKE_PREFIX.test(name) || PROTECTED.has(name)) return false; // belt and braces + try { + mcpctl(['delete', resource, name, ...extra]); + return true; + } catch (err) { + process.stderr.write(` ! ${resource} ${name}: ${err instanceof Error ? err.message.split('\n')[0] : String(err)}\n`); + return false; + } +} + +const projects = sweepable(list('projects')); +// Tokens live on real projects too (the suites mint smoke-* tokens against +// mcpctl-development and sre), so they need sweeping independently of projects. +const tokens = sweepable(list('mcptoken')); +const servers = sweepable(list('servers')); +const llms = sweepable(list('llms')); +const agents = sweepable(list('agents')); + +const groups: Array<[string, Named[], string[]]> = [ + ['project', projects, []], + ['mcptoken', tokens, []], + ['server', servers, []], + ['llm', llms, []], + ['agent', agents, []], +]; + +let total = 0; +for (const [, rows] of groups) total += rows.length; + +if (total === 0) { + process.stdout.write('smoke:clean — nothing to sweep\n'); + process.exit(0); +} + +for (const [kind, rows] of groups) { + if (rows.length === 0) continue; + process.stdout.write(`${kind}: ${String(rows.length)}\n`); + for (const r of rows.slice(0, 5)) process.stdout.write(` ${r.name ?? ''}${r.projectName ? ` (in ${r.projectName})` : ''}\n`); + if (rows.length > 5) process.stdout.write(` … and ${String(rows.length - 5)} more\n`); +} + +if (!APPLY) { + process.stdout.write(`\n${String(total)} resource(s) would be deleted. Re-run with --yes to apply.\n`); + process.exit(0); +} + +let deleted = 0; +for (const [kind, rows, extra] of groups) { + for (const r of rows) if (remove(kind, r, extra)) deleted++; +} +process.stdout.write(`\nsmoke:clean — deleted ${String(deleted)}/${String(total)}\n`); +process.exit(deleted === total ? 0 : 1); diff --git a/src/mcplocal/tests/smoke/mcptoken.smoke.test.ts b/src/mcplocal/tests/smoke/mcptoken.smoke.test.ts index 1053533..0411ae0 100644 --- a/src/mcplocal/tests/smoke/mcptoken.smoke.test.ts +++ b/src/mcplocal/tests/smoke/mcptoken.smoke.test.ts @@ -14,7 +14,7 @@ * * Run with: pnpm test:smoke */ -import { describe, it, expect, beforeAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import http from 'node:http'; import https from 'node:https'; import { execSync } from 'node:child_process'; @@ -87,9 +87,23 @@ describe('mcptoken smoke', () => { } }, 20_000); + // Cleanup belongs in afterAll, not an `it()`: a failing assertion earlier in + // the file must not strand server-side projects. Deliberately NOT gated on + // `gatewayUp` — the project is created through mcpd, which can be reachable + // even when the gateway healthz probe is not, and deleting something that was + // never created is a harmless no-op. + // + // `delete project` takes no --force (only `create project` does); passing it + // made every one of these calls exit non-zero and silently do nothing, which + // is how hundreds of smoke-* projects accumulated on the shared mcpd. + afterAll(() => { + run(`delete project ${PROJECT_NAME}`); + run(`delete project ${OTHER_PROJECT}`); + }); + it('creates the project and a project-scoped mcptoken', () => { if (!gatewayUp) return; - run(`delete project ${PROJECT_NAME} --force`); // cleanup leftovers — best-effort + run(`delete project ${PROJECT_NAME}`); // cleanup leftovers — best-effort const createProj = run(`create project ${PROJECT_NAME} --force`); expect(createProj.code).toBe(0); @@ -149,10 +163,8 @@ describe('mcptoken smoke', () => { expect(report.error ?? '').toMatch(/401|revoked|Invalid token/i); }, 20_000); - it('cleans up test fixtures', () => { + it('recorded a tool name for the gated catalogue', () => { if (!gatewayUp) return; - run(`delete project ${PROJECT_NAME} --force`); - run(`delete project ${OTHER_PROJECT} --force`); expect(knownToolName === undefined || typeof knownToolName === 'string').toBe(true); }); }); diff --git a/src/mcplocal/tests/smoke/project-llm-ref.smoke.test.ts b/src/mcplocal/tests/smoke/project-llm-ref.smoke.test.ts index 5c85d35..d442ca2 100644 --- a/src/mcplocal/tests/smoke/project-llm-ref.smoke.test.ts +++ b/src/mcplocal/tests/smoke/project-llm-ref.smoke.test.ts @@ -81,17 +81,20 @@ describe('project-llm-ref smoke', () => { } }, 30_000); + // Not gated on `mcpdUp`: if creation got far enough to leave a project + // behind, cleanup must still run. Deleting a non-existent project is a no-op. + // (`delete project` takes no --force — passing it made these calls fail + // silently and leak a project per run.) afterAll(() => { - if (!mcpdUp) return; - run(`delete project ${PROJ_OK} --force`); - run(`delete project ${PROJ_ORPHAN} --force`); - run(`delete project ${PROJ_NONE} --force`); + run(`delete project ${PROJ_OK}`); + run(`delete project ${PROJ_ORPHAN}`); + run(`delete project ${PROJ_NONE}`); run(`delete llm ${LLM_NAME}`); }); it('project with --llm pointing at a registered Llm describes without warning', () => { if (!mcpdUp) return; - run(`delete project ${PROJ_OK} --force`); + run(`delete project ${PROJ_OK}`); const created = run(`create project ${PROJ_OK} --llm ${LLM_NAME}`); expect(created.code, created.stderr || created.stdout).toBe(0); @@ -104,7 +107,7 @@ describe('project-llm-ref smoke', () => { it('project with --llm naming an unregistered Llm shows the warning line', () => { if (!mcpdUp) return; - run(`delete project ${PROJ_ORPHAN} --force`); + run(`delete project ${PROJ_ORPHAN}`); const created = run(`create project ${PROJ_ORPHAN} --llm claude-ghost-${SUFFIX}`); expect(created.code, created.stderr || created.stdout).toBe(0); @@ -117,7 +120,7 @@ describe('project-llm-ref smoke', () => { it('project with --llm none treats it as an explicit disable (no warning)', () => { if (!mcpdUp) return; - run(`delete project ${PROJ_NONE} --force`); + run(`delete project ${PROJ_NONE}`); const created = run(`create project ${PROJ_NONE} --llm none`); expect(created.code).toBe(0);