From 4813ed3f0c6d883e2978ed6009699c3ccc875938 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Thu, 30 Jul 2026 22:22:31 -0700 Subject: [PATCH] fix(db): harden embedded Postgres test start with bounded retry (#10540) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The database layer uses embedded Postgres for isolated test runs > - A port probe can fail when another process takes the same port before Postgres binds it > - That race can make a test fail even when the code under test is fine > - This pull request adds bounded retry and clearer error text to the embedded Postgres start path > - The benefit is more stable tests and faster diagnosis when startup still fails ## Linked Issues or Issue Description No public GitHub issue exists for this change. This PR fixes a flaky embedded Postgres test start path. The helper can lose a port between probe and bind. This PR retries the start with a fresh port and a fresh data directory. Related public context: - Refs #7259 - Refs #9769 ## What Changed - Add bounded retry around embedded Postgres initialization and start. - Stop each failed attempt and remove its data directory before the next attempt. - Capture Postgres output in the thrown error so the failure is easier to read. - Add unit coverage for retry success, retry exhaustion, and the improved error text. ## Verification - `pnpm --filter @paperclipai/db exec vitest run src/test-embedded-postgres.test.ts src/embedded-postgres-error.test.ts` - `pnpm --filter @paperclipai/db exec vitest run` - `pnpm --filter @paperclipai/db exec vitest run` passed in the worktree after the change. - `worktree.test.ts > quarantines copied live execution state in seeded worktree databases` passed. - A real cluster loop of 100 starts passed with 0 failures. ## Risks - The retry can hide a real startup fault until the fifth try. - The bound keeps the wait short, and the final error still shows the captured Postgres log. - This change only affects the embedded Postgres test start helper. ## Model Used OpenAI Codex, GPT-5, tool-use enabled. ## 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] 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 --- .../db/src/test-embedded-postgres.test.ts | 113 +++++++++++++++ packages/db/src/test-embedded-postgres.ts | 132 +++++++++++++----- 2 files changed, 208 insertions(+), 37 deletions(-) create mode 100644 packages/db/src/test-embedded-postgres.test.ts diff --git a/packages/db/src/test-embedded-postgres.test.ts b/packages/db/src/test-embedded-postgres.test.ts new file mode 100644 index 0000000000..1d4f78c797 --- /dev/null +++ b/packages/db/src/test-embedded-postgres.test.ts @@ -0,0 +1,113 @@ +import fs from "node:fs"; +import { afterEach, describe, expect, it } from "vitest"; +import { + __embeddedPostgresStartMaxAttemptsForTests as MAX_ATTEMPTS, + __setEmbeddedPostgresCtorProviderForTests, + __startEmbeddedPostgresWithRetryForTests as startWithRetry, +} from "./test-embedded-postgres.js"; + +// A fake embedded-postgres constructor. It records every constructed instance so +// the test can assert the retry uses a fresh port and a fresh data directory each +// attempt. `start()` emits the same output the real cluster writes for a port +// conflict, then rejects with an empty message (the real rejection shape). The +// option type matches the real constructor so no type cast is needed. +type FakeOptions = { + databaseDir: string; + user: string; + password: string; + port: number; + persistent: boolean; + initdbFlags?: string[]; + onLog?: (message: unknown) => void; + onError?: (message: unknown) => void; +}; + +const BIND_CONFLICT_LOG = 'could not bind IPv4 address "127.0.0.1": Address already in use'; + +function makeFakeCtor(failFirst: number) { + const constructed: FakeOptions[] = []; + let started = 0; + + class FakeEmbeddedPostgres { + private readonly options: FakeOptions; + constructor(options: FakeOptions) { + this.options = options; + constructed.push(options); + } + async initialise(): Promise {} + async start(): Promise { + started += 1; + if (started <= failFirst) { + // Mirror the real failure: Postgres logs the reason, then `start()` + // rejects with an Error whose message is empty. + this.options.onLog?.(BIND_CONFLICT_LOG); + throw new Error(); + } + } + async stop(): Promise {} + } + + return { ctor: FakeEmbeddedPostgres, constructed }; +} + +describe("startEmbeddedPostgresWithRetry", () => { + afterEach(() => { + __setEmbeddedPostgresCtorProviderForTests(null); + }); + + it("recovers from a transient port conflict and returns on a later attempt", async () => { + const { ctor, constructed } = makeFakeCtor(2); + __setEmbeddedPostgresCtorProviderForTests(async () => ctor); + + const started = await startWithRetry("paperclip-retry-recover-"); + + // The first two attempts fail, the third succeeds. + expect(constructed).toHaveLength(3); + + // Each attempt uses a fresh data directory. The two failed directories are + // removed. The returned directory still exists. + const dataDirs = constructed.map((options) => options.databaseDir); + expect(new Set(dataDirs).size).toBe(3); + expect(fs.existsSync(dataDirs[0])).toBe(false); + expect(fs.existsSync(dataDirs[1])).toBe(false); + expect(started.dataDir).toBe(dataDirs[2]); + expect(fs.existsSync(started.dataDir)).toBe(true); + + // Each attempt allocates a port. + for (const options of constructed) { + expect(Number.isInteger(options.port)).toBe(true); + expect(options.port).toBeGreaterThan(0); + } + + // Clean up the returned attempt. + await started.instance.stop(); + fs.rmSync(started.dataDir, { recursive: true, force: true }); + }); + + it("throws with the real Postgres output after the attempt bound", async () => { + const { ctor, constructed } = makeFakeCtor(Number.POSITIVE_INFINITY); + __setEmbeddedPostgresCtorProviderForTests(async () => ctor); + + await expect(startWithRetry("paperclip-retry-fail-")).rejects.toThrow(/after \d+ attempts/); + + // The retry stops at the bound and does not loop forever. + expect(constructed).toHaveLength(MAX_ATTEMPTS); + + // Every failed attempt removes its data directory. + for (const options of constructed) { + expect(fs.existsSync(options.databaseDir)).toBe(false); + } + }); + + it("keeps the real failure reason instead of a generic fallback", async () => { + const { ctor } = makeFakeCtor(Number.POSITIVE_INFINITY); + __setEmbeddedPostgresCtorProviderForTests(async () => ctor); + + const error = await startWithRetry("paperclip-retry-reason-").catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(Error); + // The thrown message carries the captured Postgres output, not only the + // generic "embedded Postgres startup failed" text. + expect((error as Error).message).toContain("Address already in use"); + }); +}); diff --git a/packages/db/src/test-embedded-postgres.ts b/packages/db/src/test-embedded-postgres.ts index d88ca59c37..e85fd1a721 100644 --- a/packages/db/src/test-embedded-postgres.ts +++ b/packages/db/src/test-embedded-postgres.ts @@ -3,6 +3,10 @@ import net from "node:net"; import os from "node:os"; import path from "node:path"; import { applyPendingMigrations, ensurePostgresDatabase } from "./client.js"; +import { + createEmbeddedPostgresLogBuffer, + formatEmbeddedPostgresError, +} from "./embedded-postgres-error.js"; import { prepareEmbeddedPostgresNativeRuntime } from "./embedded-postgres-native.js"; type EmbeddedPostgresInstance = { @@ -47,12 +51,29 @@ function getReservedTestPorts(): Set { return new Set(configuredPorts.filter((port) => Number.isInteger(port) && port > 0 && port <= 65535)); } -async function getEmbeddedPostgresCtor(): Promise { +type EmbeddedPostgresCtorProvider = () => Promise; + +async function loadEmbeddedPostgresCtor(): Promise { 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 { + return await embeddedPostgresCtorProvider(); +} + async function getAvailablePort(): Promise { const reservedPorts = getReservedTestPorts(); for (let attempt = 0; attempt < 20; attempt += 1) { @@ -88,6 +109,11 @@ 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", @@ -95,11 +121,11 @@ async function createEmbeddedPostgresTestInstance(tempDirPrefix: string) { port, persistent: true, initdbFlags: ["--encoding=UTF8", "--locale=C", "--lc-messages=C"], - onLog: () => {}, - onError: () => {}, + onLog: (message) => logBuffer.append(message), + onError: (message) => logBuffer.append(message), }); - return { dataDir, port, instance }; + return { dataDir, port, instance, getRecentLogs: () => logBuffer.getRecentLogs() }; } function cleanupEmbeddedPostgresTestDirs(dataDir: string) { @@ -161,34 +187,72 @@ async function stopEmbeddedPostgresBounded( } } -function formatEmbeddedPostgresError(error: unknown): string { - if (error instanceof Error && error.message.length > 0) return error.message; - if (typeof error === "string" && error.length > 0) return error; - return "embedded Postgres startup failed"; +// 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 { - let dataDir: string | null = null; - let instance: EmbeddedPostgresInstance | null = null; + let started: { dataDir: string; instance: EmbeddedPostgresInstance } | null = null; try { - const created = await createEmbeddedPostgresTestInstance( - "paperclip-embedded-postgres-probe-", - ); - dataDir = created.dataDir; - instance = created.instance; - await instance.initialise(); - await instance.start(); + started = await startEmbeddedPostgresWithRetry("paperclip-embedded-postgres-probe-"); return { supported: true }; } catch (error) { return { supported: false, - reason: formatEmbeddedPostgresError(error), + reason: formatEmbeddedPostgresError(error, { + fallbackMessage: "embedded Postgres startup failed", + }).message, }; } finally { - await stopEmbeddedPostgresBounded(instance, () => { - if (dataDir) cleanupEmbeddedPostgresTestDirs(dataDir); - }); + if (started) { + const { dataDir, instance } = started; + await stopEmbeddedPostgresBounded(instance, () => cleanupEmbeddedPostgresTestDirs(dataDir)); + } } } @@ -202,17 +266,11 @@ export async function getEmbeddedPostgresTestSupport(): Promise { - let dataDir: string | null = null; - let instance: EmbeddedPostgresInstance | null = null; + // 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 created = await createEmbeddedPostgresTestInstance(tempDirPrefix); - dataDir = created.dataDir; - instance = created.instance; - const { port } = created; - await instance.initialise(); - await instance.start(); - 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`; @@ -221,17 +279,17 @@ export async function startEmbeddedPostgresTestDatabase( return { connectionString, cleanup: async () => { - await stopEmbeddedPostgresBounded(instance, () => { - if (dataDir) cleanupEmbeddedPostgresTestDirs(dataDir); - }); + await stopEmbeddedPostgresBounded(instance, () => cleanupEmbeddedPostgresTestDirs(dataDir)); }, }; } catch (error) { - await stopEmbeddedPostgresBounded(instance, () => { - if (dataDir) cleanupEmbeddedPostgresTestDirs(dataDir); - }); + await stopEmbeddedPostgresBounded(instance, () => cleanupEmbeddedPostgresTestDirs(dataDir)); throw new Error( - `Failed to start embedded PostgreSQL test database: ${formatEmbeddedPostgresError(error)}`, + `Failed to start embedded PostgreSQL test database: ${ + formatEmbeddedPostgresError(error, { + fallbackMessage: "embedded Postgres startup failed", + }).message + }`, ); } }