Files
PaperClipAI/packages/db/src/test-embedded-postgres.ts
T
Nicky LeachandPaperclip a3b78f9d34 fix(cli): make embedded-Postgres tests survive runner contention (#12466)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - The CLI test suite uses embedded Postgres for worktree checks.
> - Several tests start a cluster and run migrations on each test.
> - Runner contention can make this cost class exceed small hand-written
time budgets.
> - A timed-out test can also leave the cluster alive while cleanup
removes its data directory.
> - This pull request gives the cost class one measured timeout and
registers cleanup for the timeout path.
> - The benefit is stable tests and fewer orphaned embedded Postgres
processes.

## Linked Issues or Issue Description

No public GitHub issue tracks this defect. Related PRs #10987 and #10961
address wider embedded-Postgres test setup issues. This pull request
addresses the CLI timeout and cleanup path described below.

**What happened?**
Five CLI tests used different hand-written time budgets while they
started embedded Postgres and ran migrations. Runner contention made one
test exceed its budget. A timeout also skipped the `finally` cleanup
path and left a Postgres process alive.

**Expected behavior**
Each embedded-Postgres test gets a budget that covers measured runner
contention. Cleanup stops the cluster before the test removes its data
directory, including after a timeout.

**Steps to reproduce**
1. Run `npx vitest run cli/src/__tests__/worktree.test.ts` on a
contended runner.
2. Observe the embedded-Postgres tests take longer than their small
hand-written budgets.
3. Inspect the process list after a timeout and observe an orphaned
Postgres process.

**Paperclip version or commit**
The defect reproduces on `master` before this pull request.

**Deployment mode**
Built from source with Vitest.

**Installation method**
Built from source with pnpm.

## What Changed

- Export `EMBEDDED_POSTGRES_TEST_TIMEOUT_MS` from the embedded-Postgres
test helper.
- Apply the shared timeout to every test defined by
`itEmbeddedPostgres`.
- Replace the five hand-written timeout values in `worktree.test.ts`.
- Register temporary-directory removal and cluster stop with
`onTestFinished` in the correct order.
- Restore the working directory during timeout cleanup.

## Verification

- `npx vitest run cli/src/__tests__/worktree.test.ts` passes 63 tests.
- `npx vitest run cli/src` passes 424 tests across 59 files.
- `tsc --noEmit` in `cli/` adds no new error in the touched files.
- CI must pass on the pull request head.
- Greptile must report 5/5 with no unresolved blocking thread.

## Risks

- Low risk. The change affects test helpers and test cleanup only.
- The shared budget can lengthen a failing test before Vitest reports
the failure.
- The cleanup order depends on Vitest callback order, which the tests
now use explicitly.

## Model Used

OpenAI Codex, GPT-5. The model used tool calls and code review support.
The context window size and internal reasoning mode were not exposed in
this run.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` /
`Closes: #` / `Refs: #` OR (b) described the issue in-PR following the
relevant issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [x] All Paperclip CI gates are green
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge

---------

Co-authored-by: Paperclip <noreply@paperclip.ing>
2026-08-28 13:51:54 -07:00

310 lines
12 KiB
TypeScript

import fs from "node:fs";
import net from "node:net";
import os from "node:os";
import path from "node:path";
import { applyPendingMigrations, closeRegisteredClients, ensurePostgresDatabase } from "./client.js";
import {
createEmbeddedPostgresLogBuffer,
formatEmbeddedPostgresError,
} from "./embedded-postgres-error.js";
import { prepareEmbeddedPostgresNativeRuntime } from "./embedded-postgres-native.js";
// Time budget (ms) for a vitest test in the embedded-Postgres cost class: a
// test that starts an embedded Postgres cluster and runs migrations. Measured
// evidence: this cost class normally finishes in well under 10s. Under a
// contended CI runner the same test took up to 4.9x longer. This budget
// gives about 10x headroom over the clean time, so a contended run still
// passes while a genuine hang still fails fast.
export const EMBEDDED_POSTGRES_TEST_TIMEOUT_MS = 90_000;
type EmbeddedPostgresInstance = {
initialise(): Promise<void>;
start(): Promise<void>;
stop(): Promise<void>;
};
type EmbeddedPostgresCtor = new (opts: {
databaseDir: string;
user: string;
password: string;
port: number;
persistent: boolean;
initdbFlags?: string[];
onLog?: (message: unknown) => void;
onError?: (message: unknown) => void;
}) => EmbeddedPostgresInstance;
export type EmbeddedPostgresTestSupport = {
supported: boolean;
reason?: string;
};
export type EmbeddedPostgresTestDatabase = {
connectionString: string;
cleanup(): Promise<void>;
};
let embeddedPostgresSupportPromise: Promise<EmbeddedPostgresTestSupport> | null = null;
const DEFAULT_PAPERCLIP_EMBEDDED_POSTGRES_PORT = 54329;
function getReservedTestPorts(): Set<number> {
const configuredPorts = [
DEFAULT_PAPERCLIP_EMBEDDED_POSTGRES_PORT,
Number.parseInt(process.env.PAPERCLIP_EMBEDDED_POSTGRES_PORT ?? "", 10),
...String(process.env.PAPERCLIP_TEST_POSTGRES_RESERVED_PORTS ?? "")
.split(",")
.map((value) => Number.parseInt(value.trim(), 10)),
];
return new Set(configuredPorts.filter((port) => Number.isInteger(port) && port > 0 && port <= 65535));
}
type EmbeddedPostgresCtorProvider = () => Promise<EmbeddedPostgresCtor>;
async function loadEmbeddedPostgresCtor(): Promise<EmbeddedPostgresCtor> {
const mod = await import("embedded-postgres");
await prepareEmbeddedPostgresNativeRuntime();
return mod.default as EmbeddedPostgresCtor;
}
let embeddedPostgresCtorProvider: EmbeddedPostgresCtorProvider = loadEmbeddedPostgresCtor;
// Test seam. Replace the embedded-postgres constructor provider so a test can
// simulate a failed start without the native runtime. Pass `null` to restore
// the default provider. This module is test support only, so the seam is safe.
export function __setEmbeddedPostgresCtorProviderForTests(
provider: EmbeddedPostgresCtorProvider | null,
): void {
embeddedPostgresCtorProvider = provider ?? loadEmbeddedPostgresCtor;
}
async function getEmbeddedPostgresCtor(): Promise<EmbeddedPostgresCtor> {
return await embeddedPostgresCtorProvider();
}
async function getAvailablePort(): Promise<number> {
const reservedPorts = getReservedTestPorts();
for (let attempt = 0; attempt < 20; attempt += 1) {
const port = await new Promise<number>((resolve, reject) => {
const server = net.createServer();
server.unref();
server.on("error", reject);
server.listen(0, "127.0.0.1", () => {
const address = server.address();
if (!address || typeof address === "string") {
server.close(() => reject(new Error("Failed to allocate test port")));
return;
}
const { port } = address;
server.close((error) => {
if (error) reject(error);
else resolve(port);
});
});
});
if (!reservedPorts.has(port)) return port;
}
throw new Error(
`Failed to allocate embedded Postgres test port outside reserved Paperclip ports: ${[
...reservedPorts,
].join(", ")}`,
);
}
async function createEmbeddedPostgresTestInstance(tempDirPrefix: string) {
const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), tempDirPrefix));
const port = await getAvailablePort();
const EmbeddedPostgres = await getEmbeddedPostgresCtor();
// Postgres writes the true reason for a failed start to its output, for
// example `could not bind IPv4 address "127.0.0.1": Address already in use`.
// The `start()` rejection carries an empty message, so we capture the output
// in a bounded buffer and surface it in the thrown error.
const logBuffer = createEmbeddedPostgresLogBuffer();
const instance = new EmbeddedPostgres({
databaseDir: dataDir,
user: "paperclip",
password: "paperclip",
port,
persistent: true,
initdbFlags: ["--encoding=UTF8", "--locale=C", "--lc-messages=C"],
onLog: (message) => logBuffer.append(message),
onError: (message) => logBuffer.append(message),
});
return { dataDir, port, instance, getRecentLogs: () => logBuffer.getRecentLogs() };
}
function cleanupEmbeddedPostgresTestDirs(dataDir: string) {
fs.rmSync(dataDir, { recursive: true, force: true });
}
// Upper bound (ms) on how long we wait for the embedded Postgres cluster to
// stop gracefully before abandoning the wait and returning from the hook.
const EMBEDDED_POSTGRES_STOP_TIMEOUT_MS = 5000;
// `embedded-postgres@18.1.0-beta.16` exposes only `stop(): Promise<void>` — no
// shutdown-mode argument. Internally it SIGINTs the postgres process (already
// PostgreSQL "fast shutdown") and resolves *only* on the child's `exit` event,
// with no time bound of its own. Under the loaded serial server shard a slow
// shutdown checkpoint can push that past vitest's hookTimeout and hang the
// afterAll hook. So we bound the graceful stop: if it overruns, we stop waiting
// and return so the hook completes. The SIGINT has already been delivered, so
// the abandoned process still exits on its own (and again when the runner exits).
// Errors are swallowed, matching prior behavior.
//
// `cleanupFn` (data-dir reclaim) is chained on the raw `stop()` promise, not on
// the timeout race, so the disposable data dir is removed *only after* `stop()`
// actually settles — i.e. once the child Postgres process has exited. Removing
// it on the timeout path would pull the data files out from under a still-running
// cluster and provoke checkpoint / WAL I/O errors. In the fast path `cleanupFn`
// has run by the time this resolves; in the timeout path it runs asynchronously
// once the abandoned process finally exits.
async function stopEmbeddedPostgresBounded(
instance: EmbeddedPostgresInstance | null,
cleanupFn?: () => void,
): Promise<void> {
if (!instance) {
cleanupFn?.();
return;
}
let timer: ReturnType<typeof setTimeout> | undefined;
const stopped = instance
.stop()
.catch(() => {
// Swallow shutdown errors — the data dir is reclaimed regardless.
})
.finally(() => {
try {
cleanupFn?.();
} catch {
// Best-effort reclaim; ignore removal errors.
}
});
try {
await Promise.race([
stopped,
new Promise<void>((resolve) => {
timer = setTimeout(resolve, EMBEDDED_POSTGRES_STOP_TIMEOUT_MS);
timer.unref?.();
}),
]);
} finally {
if (timer) clearTimeout(timer);
}
}
// Upper bound on start attempts. `getAvailablePort` uses a check-then-use probe:
// it binds port 0, reads the assigned port, closes the probe, then Postgres binds
// that port. Under load another process can take the port in that window, so the
// bind fails with "Address already in use" and `start()` rejects. Each retry uses
// a fresh port and a fresh data directory, so a transient collision clears.
const EMBEDDED_POSTGRES_START_MAX_ATTEMPTS = 5;
// Start one embedded Postgres cluster with a bounded retry. Each attempt gets a
// fresh port and a fresh data directory. On a failed attempt we stop the cluster
// and remove its data directory before the next attempt. After the last attempt
// we throw with the real Postgres output so the failure is loud and diagnosable.
async function startEmbeddedPostgresWithRetry(tempDirPrefix: string): Promise<{
port: number;
dataDir: string;
instance: EmbeddedPostgresInstance;
}> {
let lastError = new Error("embedded Postgres startup failed");
for (let attempt = 1; attempt <= EMBEDDED_POSTGRES_START_MAX_ATTEMPTS; attempt += 1) {
const created = await createEmbeddedPostgresTestInstance(tempDirPrefix);
try {
await created.instance.initialise();
await created.instance.start();
return { port: created.port, dataDir: created.dataDir, instance: created.instance };
} catch (error) {
lastError = formatEmbeddedPostgresError(error, {
fallbackMessage: "embedded Postgres startup failed",
recentLogs: created.getRecentLogs(),
});
// Stop the failed cluster and remove its data directory. The next attempt
// allocates a fresh port and a fresh data directory.
await stopEmbeddedPostgresBounded(created.instance, () =>
cleanupEmbeddedPostgresTestDirs(created.dataDir),
);
}
}
throw new Error(
`Failed to start embedded PostgreSQL test database after ${EMBEDDED_POSTGRES_START_MAX_ATTEMPTS} attempts: ${lastError.message}`,
);
}
// Test-only accessors. Production callers use `startEmbeddedPostgresTestDatabase`
// or `getEmbeddedPostgresTestSupport`. A test drives the bounded retry directly
// so it does not need a real Postgres connection.
export const __startEmbeddedPostgresWithRetryForTests = startEmbeddedPostgresWithRetry;
export const __embeddedPostgresStartMaxAttemptsForTests = EMBEDDED_POSTGRES_START_MAX_ATTEMPTS;
async function probeEmbeddedPostgresSupport(): Promise<EmbeddedPostgresTestSupport> {
let started: { dataDir: string; instance: EmbeddedPostgresInstance } | null = null;
try {
started = await startEmbeddedPostgresWithRetry("paperclip-embedded-postgres-probe-");
return { supported: true };
} catch (error) {
return {
supported: false,
reason: formatEmbeddedPostgresError(error, {
fallbackMessage: "embedded Postgres startup failed",
}).message,
};
} finally {
if (started) {
const { dataDir, instance } = started;
await stopEmbeddedPostgresBounded(instance, () => cleanupEmbeddedPostgresTestDirs(dataDir));
}
}
}
export async function getEmbeddedPostgresTestSupport(): Promise<EmbeddedPostgresTestSupport> {
if (!embeddedPostgresSupportPromise) {
embeddedPostgresSupportPromise = probeEmbeddedPostgresSupport();
}
return await embeddedPostgresSupportPromise;
}
export async function startEmbeddedPostgresTestDatabase(
tempDirPrefix: string,
): Promise<EmbeddedPostgresTestDatabase> {
// The bounded retry hardens the cluster start against the port race. It throws
// with the real Postgres output if every attempt fails.
const { port, dataDir, instance } = await startEmbeddedPostgresWithRetry(tempDirPrefix);
try {
const adminConnectionString = `postgres://paperclip:paperclip@127.0.0.1:${port}/postgres`;
await ensurePostgresDatabase(adminConnectionString, "paperclip");
const connectionString = `postgres://paperclip:paperclip@127.0.0.1:${port}/paperclip`;
await applyPendingMigrations(connectionString);
return {
connectionString,
cleanup: async () => {
// End every client a caller created against this cluster first. A
// client that still holds a reserved connection when the cluster
// stops can crash the process: the stop kills the backend socket,
// but a queued write on that connection still fires later and finds
// a null socket.
await closeRegisteredClients(connectionString);
await stopEmbeddedPostgresBounded(instance, () => cleanupEmbeddedPostgresTestDirs(dataDir));
},
};
} catch (error) {
await stopEmbeddedPostgresBounded(instance, () => cleanupEmbeddedPostgresTestDirs(dataDir));
throw new Error(
`Failed to start embedded PostgreSQL test database: ${
formatEmbeddedPostgresError(error, {
fallbackMessage: "embedded Postgres startup failed",
}).message
}`,
);
}
}