diff --git a/docs/solutions/test-failures/postgres-reliability-helper-ddl-audit.md b/docs/solutions/test-failures/postgres-reliability-helper-ddl-audit.md new file mode 100644 index 0000000000..d3ed0b19d3 --- /dev/null +++ b/docs/solutions/test-failures/postgres-reliability-helper-ddl-audit.md @@ -0,0 +1,48 @@ +--- +category: test-failures +module: testing +problem_type: PostgreSQL fixture cleanup and DDL contention +applies_when: Engine reliability tests create isolated PostgreSQL databases under loaded worker fan-out. +tags: [postgresql, testing, reliability, ddl, cleanup] +--- + +# PostgreSQL reliability helper DDL audit + +FN-9133 audited `packages/engine/src/__tests__/reliability-interactions/_helpers.ts`, the contained PostgreSQL fixture helper used by the engine reliability lane. + +## Inventory + +Before the fix, each fixture used `psql ... -f -` for all database DDL, issued a redundant unique-name `DROP DATABASE IF EXISTS` before `CREATE DATABASE`, and used a best-effort cleanup drop without `WITH (FORCE)`. That was three explicit database statements per fixture (drop/create/drop), plus a full schema baseline. The helper's runtime pool permits five connections, so a live connection could make the cleanup drop fail and leave a `fusion_rel_%` database. + +The audit's source census found 53 `makeReliabilityFixture(`, 3 `makePgTaskStore(`, and 3 `createPgLayer(` occurrences (including declarations) in this checkout. A repository scan for `CREATE DATABASE` across engine, dashboard, and CLI sources returned only this helper, so the divergence was contained. + +## Measurements + +Each measurement ran `engine-reliability` with retained output, then queried `pg_database` for `fusion_rel_%` and used `pgrep -x psql` for the child census. + +| phase/run | workers | result | wall seconds | leaked databases | live psql | +|---|---|---|---:|---:|---:| +| baseline default | computed default | 100 files / 548 tests passed | 52.85 | 1 | 0 | +| baseline elevated 1 | 12 | passed | 38.62 | 0 | 0 | +| baseline elevated 2 | 12 | passed | 35.20 | 0 | 0 | +| post default | computed default | passed | 49.01 | 0 | 0 | +| post elevated 1 | 12 | passed | 45.77 | 0 | 0 | +| post elevated 2 | 12 | passed | 48.71 | 0 | 0 | + +Baseline variation was 35.20/38.62/52.85 seconds (min/median/max); post-change variation was 45.77/48.71/49.01. This is not a timing-improvement claim. The acceptance band was three green post-change runs, zero database leaks, zero psql children, and no run slower than the 52.85-second baseline maximum; all runs met it. + +## Verdict + +### Admission/deferred drain: do not copy + +FN-9130 measured uniform and drop-only K=4 in-hook admission as regressions, with unstable watchdog counts even on unchanged ungated code. FN-9133 therefore does **not** add an advisory admission gate or deferred drain. FN-9134 owns the open structural alternative. + +### Maintenance-connection contract: conform + +The helper now uses a short-lived owned `postgres.js` maintenance client with a server-side statement timeout, a JavaScript deadline, and forced socket close. It no longer needs a `psql` binary gate. Cleanup uses `DROP DATABASE IF EXISTS ... WITH (FORCE)`, and the redundant pre-create drop is removed; the explicit DDL minimum is now create plus forced cleanup drop. + +The regression test holds an independent open transaction, runs helper cleanup, verifies that `pg_database` no longer contains the fixture, and calls cleanup twice. A temporary local revert failed the test with the retained `fusion_rel_74100_1_dobb29` database. The test also pins the absence of `exec`, `execSync`, or `spawnSync` psql DDL launch sites. + +## Policy preservation + +No test or hook timeout, worker/fan-out setting, `exclude`, retry, `.skip` policy (other than the standard `hasPg` conditional test gate), or quarantine ledger changed. The changed lane, targeted contract test, lint, `pnpm verify:fast`, and `pnpm build` passed. diff --git a/docs/testing.md b/docs/testing.md index 689873491f..db1771cba5 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -23,7 +23,7 @@ Gate membership is the explicit allow-list in `packages/engine/vitest.config.ts` **PostgreSQL and unit gate policy:** `packages/core`'s `test:pg-gate` intentionally runs `handoff-to-review-atomicity.pg.test.ts` and `task-lifecycle-e2e.pg.test.ts`, preserving atomic-handoff and lifecycle real-backend canaries. `sync-workflow-ir-is-always-default.pg.test.ts` was evicted under the merge-gate flake rule; its coverage remains in the non-blocking core suite (see the [observed suite-only flakes register](solutions/test-failures/suite-only-flakes-observed-register.md#6-sync-workflow-ir-default-canary-setup-hook)). `test:unit-gate` runs `task-merge.test.ts`, `legacy-adoption.test.ts`, `no-hardcoded-lifecycle-columns.test.ts`, and `sync-workflow-ir-callsite-allowlist.test.ts`. Every other former PG gate member remains enabled and discovered by the non-blocking command `pnpm --filter @fusion/core test` (default config: `src/**/*.test.ts`, no PG quarantine exclusions). `scripts/__tests__/engine-vitest-gate-policy.test.mjs` pins the exact two PG and four unit files, all waits, CI-shape ordering, and every engine/static member. -**PostgreSQL fixture bootstrap:** Do not hand-roll per-file `CREATE DATABASE` or drop helpers. Use `createEmptyPgTestDatabase` for migration-application and upgrade contracts, and `createBaselinedPgTestDatabase` only when an already-applied schema is the intended fixture state. Both keep database lifecycle and cleanup behavior aligned with the shared harness. +**PostgreSQL fixture bootstrap:** Do not hand-roll per-file `CREATE DATABASE` or drop helpers. Use `createEmptyPgTestDatabase` for migration-application and upgrade contracts, and `createBaselinedPgTestDatabase` only when an already-applied schema is the intended fixture state. Both keep database lifecycle and cleanup behavior aligned with the shared harness. Engine reliability fixtures follow the same bounded maintenance-connection and forced-cleanup contract; see [the reliability helper DDL audit](solutions/test-failures/postgres-reliability-helper-ddl-audit.md). **Gate-safe `@fusion/core` barrel:** the `engine-core` project resolves `@fusion/core` to `packages/core/src/index.gate.ts` (a project-scoped `resolve.alias`, not the root map), not the full `packages/core/src/index.ts` barrel. `index.gate.ts` is a byte-for-byte copy of the full barrel minus the `export ... from` statements for modules added to the barrel after the last re-audit baseline — i.e. it re-exports everything the full barrel does except genuinely new, gate-irrelevant feature modules (diffed against the prior baseline commit's barrel, not hand-picked from what gate *test* files import — production modules under test pull in far more of the barrel transitively than their own imports suggest). `engine-default`/`engine-reliability`/`engine-slow` are unaffected and keep resolving the full barrel. `@fusion/engine` is untouched (no gate file imports it). When adding a new barrel module that no gate test needs, mirror the exclusion in `index.gate.ts` rather than letting gate wall-time grow — see the FNXC comment at the top of `index.gate.ts` and `packages/engine/vitest.config.ts`'s `engine-core` project for the audit procedure. diff --git a/packages/engine/package.json b/packages/engine/package.json index ceac2a96e7..9189b160a4 100644 --- a/packages/engine/package.json +++ b/packages/engine/package.json @@ -46,14 +46,15 @@ "cron-parser": "^5.5.0", "esbuild": "^0.25.12", "node-pty": "npm:@homebridge/node-pty-prebuilt-multiarch@^0.13.1", + "playwright-core": "^1.60.0", "proper-lockfile": "^4.1.2", - "typebox": "^1.0.0", - "playwright-core": "^1.60.0" + "typebox": "^1.0.0" }, "devDependencies": { "@types/node": "^25.5.0", "@types/proper-lockfile": "^4.1.4", "@vitest/coverage-v8": "^4.1.10", + "postgres": "3.4.9", "typescript": "^5.7.0", "vitest": "^4.1.10" }, diff --git a/packages/engine/src/__tests__/reliability-interactions/_helpers-pg-ddl-contract.pg.test.ts b/packages/engine/src/__tests__/reliability-interactions/_helpers-pg-ddl-contract.pg.test.ts new file mode 100644 index 0000000000..883fa09087 --- /dev/null +++ b/packages/engine/src/__tests__/reliability-interactions/_helpers-pg-ddl-contract.pg.test.ts @@ -0,0 +1,66 @@ +/* +FNXC:ReliabilityFixtures 2026-08-16-22:30: +FN-9133 pins the reliability helper's bounded maintenance-connection contract. +A live extra database connection must not survive cleanup, and DDL must never +create a psql child that can outlive Vitest's subprocess guard. +*/ +import { readFile } from "node:fs/promises"; +import { describe, expect, it } from "vitest"; +import { createConnectionSetFromUrl, drizzleSql, type ResolvedBackend } from "@fusion/core"; +import postgres from "postgres"; + +import { createPgLayer, hasPg } from "./_helpers.js"; + +const PG_TEST_URL_BASE = process.env.FUSION_PG_TEST_URL_BASE ?? "postgresql://localhost:5432"; + +function maintenanceUrl(): string { + const url = new URL(PG_TEST_URL_BASE); + url.pathname = "/postgres"; + return url.toString(); +} + +function connectionBackend(url: string): ResolvedBackend { + return { + mode: "external", + runtimeUrl: url, + migrationUrl: url, + migrationUrlOverridden: false, + }; +} + +const pgDescribe = hasPg ? describe : describe.skip; + +pgDescribe("FN-9133 reliability PostgreSQL DDL contract", () => { + it("force-drops a live fixture database, is idempotent, and has no psql DDL spawn", async () => { + const fixture = await createPgLayer(); + const heldClient = postgres(`${PG_TEST_URL_BASE}/${fixture.dbName}`, { + max: 1, + prepare: false, + onnotice: () => {}, + }); + const heldConnection = await heldClient.reserve(); + const maint = await createConnectionSetFromUrl(connectionBackend(maintenanceUrl()), { + poolMax: 1, + connectTimeoutSeconds: 5, + }); + + try { + // A reserved connection with an open transaction stays attached until FORCE ends it. + await heldConnection.unsafe("BEGIN"); + await expect(fixture.cleanup()).resolves.toBeUndefined(); + await expect(fixture.cleanup()).resolves.toBeUndefined(); + const rows = await maint.runtime.execute(drizzleSql` + SELECT datname FROM pg_database WHERE datname = ${fixture.dbName} + `); + expect(rows).toEqual([]); + const source = await readFile(new URL("./_helpers.ts", import.meta.url), "utf8"); + expect(source).not.toMatch(/(?:exec|execSync|spawnSync)\(\s*["'`]psql\b/); + } finally { + // FORCE may already have closed this socket; do not issue a follow-up query on it. + heldConnection.release(); + void heldClient.end({ timeout: 0 }).catch(() => {}); + await maint.close().catch(() => {}); + await fixture.cleanup(); + } + }); +}); diff --git a/packages/engine/src/__tests__/reliability-interactions/_helpers.ts b/packages/engine/src/__tests__/reliability-interactions/_helpers.ts index 223355b605..f1c3afb313 100644 --- a/packages/engine/src/__tests__/reliability-interactions/_helpers.ts +++ b/packages/engine/src/__tests__/reliability-interactions/_helpers.ts @@ -1,7 +1,8 @@ import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { execSync, spawnSync, exec } from "node:child_process"; +import { execSync, spawnSync } from "node:child_process"; +import postgres from "postgres"; import { Worker } from "node:worker_threads"; import { AgentStore, DEFAULT_SETTINGS, TaskStore, type Settings, type Task, @@ -94,40 +95,51 @@ function probeTcpReachable(host: string, port: number, timeoutMs = 1500): boolea } /* -FNXC:PgTestGuard 2026-07-14-07:10: -hasPg must verify BOTH that the PostgreSQL server is TCP-reachable AND that the -psql CLI binary is installed. adminExecAsync() shells out to psql for DDL -(CREATE/DROP DATABASE). Without this check, a runner with Postgres reachable -but psql missing would pass the gate and fail inside fixture creation with -spawn ENOENT instead of skipping cleanly. -*/ -const hasPsql = spawnSync("psql", ["--version"], { stdio: "pipe" }).status === 0; +FNXC:ReliabilityFixtures 2026-08-16-22:30: +FN-9133 aligns reliability-fixture DDL with the core harness after its documented +2026-07-18 orphaned `psql -f -` subprocess-guard incident. PostgreSQL reachability +is sufficient now that an owned postgres.js maintenance connection runs DDL; never +reintroduce a psql binary requirement or shell child for CREATE/DROP DATABASE. -export const hasPg = process.env.FUSION_PG_TEST_SKIP !== "1" && hasPsql && (() => { +FN-9133 deliberately does not copy an advisory admission gate or deferred drain. +FN-9130 measured those in-hook designs as regressions, and FN-9134 owns the open +contention alternative. +*/ +export const hasPg = process.env.FUSION_PG_TEST_SKIP !== "1" && (() => { if (!PG_TEST_URL_BASE) return false; const { host, port } = parseProbeTarget(PG_TEST_URL_BASE); return probeTcpReachable(host, port); })(); -function adminExecAsync(statement: string, timeoutMs = 15_000): Promise { - const { promise, resolve, reject } = Promise.withResolvers(); - const maintUrl = new URL(PG_TEST_URL_BASE); - maintUrl.pathname = "/postgres"; - const child = exec( - `psql "${maintUrl.toString()}" -v ON_ERROR_STOP=1 -f -`, - { stdio: ["pipe", "pipe", "pipe"], env: process.env, timeout: timeoutMs }, - (error, _stdout, stderr) => { - if (error) { - reject(new Error(`adminExec psql failed: ${error.message}\nstderr: ${stderr}`)); - return; - } - resolve(); - }, - ); - if (child.stdin) { - child.stdin.end(statement); +async function adminExecAsync(statement: string, timeoutMs = 15_000): Promise { + let timedOut = false; + let timeoutHandle: ReturnType | undefined; + let client: ReturnType | undefined; + try { + await Promise.race([ + (async () => { + const maintUrl = new URL(PG_TEST_URL_BASE); + maintUrl.pathname = "/postgres"; + client = postgres(maintUrl.toString(), { max: 1, prepare: false, onnotice: () => {} }); + // Cancel on the server before the JS deadline and own the socket for force-close. + await client.unsafe(`SET statement_timeout = ${Math.max(1_000, timeoutMs - 500)}`); + await client.unsafe(statement); + })(), + new Promise((_, reject) => { + timeoutHandle = setTimeout(() => { + timedOut = true; + void client?.end({ timeout: 0 }).catch(() => {}); + reject(new Error(`adminExec timed out after ${timeoutMs}ms: ${statement}`)); + }, timeoutMs); + }), + ]); + } catch (error) { + if (timedOut) throw error; + throw new Error(`adminExec failed: ${error instanceof Error ? error.message : String(error)}\nstatement: ${statement}`); + } finally { + if (timeoutHandle) clearTimeout(timeoutHandle); + await client?.end({ timeout: 5 }).catch(() => {}); } - return promise; } let relDbCounter = 0; @@ -140,17 +152,13 @@ export type PgLayerFixture = { /** * Create one isolated PostgreSQL schema layer for a reliability test. - * Callers must use {@link hasPg} before invoking this helper because DDL uses - * the `psql` binary as well as a TCP-reachable PostgreSQL server. + * Callers must use {@link hasPg} before invoking this helper because DDL needs + * a TCP-reachable PostgreSQL maintenance database. */ export async function createPgLayer(): Promise { relDbCounter += 1; const dbName = `fusion_rel_${process.pid}_${relDbCounter}_${Math.random().toString(36).slice(2, 8)}`; - try { - await adminExecAsync(`DROP DATABASE IF EXISTS "${dbName}"`); - } catch { - // may not exist — safe to ignore - } + // The pid/counter/random name is new for every fixture; a pre-create DROP only adds contention. await adminExecAsync(`CREATE DATABASE "${dbName}"`); const testUrl = `${PG_TEST_URL_BASE}/${dbName}`; const backend: ResolvedBackend = { @@ -184,15 +192,24 @@ export async function createPgLayer(): Promise { layer, dbName, cleanup: async () => { - try { await layer.close(); } catch { /* best-effort */ } - try { await adminExecAsync(`DROP DATABASE IF EXISTS "${dbName}"`); } catch { /* best-effort */ } + try { + await layer.close(); + } catch (error) { + console.warn(`[reliability-fixtures] failed to close ${dbName}`, error); + } + try { + await adminExecAsync(`DROP DATABASE IF EXISTS "${dbName}" WITH (FORCE)`); + } catch (error) { + // Preserve best-effort teardown without concealing a persistent database leak. + console.warn(`[reliability-fixtures] failed to drop ${dbName}`, error); + } }, }; } /* FNXC:PgMigrationQuarantine 2026-07-16-10:30: -VAL-REMOVAL-005 removed AgentStore's SQLite runtime path, so multi-node claim and handoff tests must construct TaskStore and every sibling AgentStore with one shared AsyncDataLayer. Reliability callers gate with hasGit && hasPg; integration callers compose hasPg ? pgDescribe : describe.skip because DDL requires both reachable PostgreSQL and psql, but integration tests do not require Git. +VAL-REMOVAL-005 removed AgentStore's SQLite runtime path, so multi-node claim and handoff tests must construct TaskStore and every sibling AgentStore with one shared AsyncDataLayer. Reliability callers gate with hasGit && hasPg; integration callers compose hasPg ? pgDescribe : describe.skip because DDL requires reachable PostgreSQL, but integration tests do not require Git. */ export async function makePgTaskStore(): Promise<{ rootDir: string; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3cf3ee7801..0da4a0e805 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -607,6 +607,9 @@ importers: '@vitest/coverage-v8': specifier: ^4.1.10 version: 4.1.10(vitest@4.1.10) + postgres: + specifier: 3.4.9 + version: 3.4.9 typescript: specifier: ^5.7.0 version: 5.9.3