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 <noreply@paperclip.ing>
This commit is contained in:
Nicky Leach
GitHub
parent
6a3cbe1c58
commit
4813ed3f0c
|
|
@ -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<void> {}
|
||||
async start(): Promise<void> {
|
||||
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<void> {}
|
||||
}
|
||||
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
|
@ -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<number> {
|
|||
return new Set(configuredPorts.filter((port) => Number.isInteger(port) && port > 0 && port <= 65535));
|
||||
}
|
||||
|
||||
async function getEmbeddedPostgresCtor(): Promise<EmbeddedPostgresCtor> {
|
||||
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) {
|
||||
|
|
@ -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<EmbeddedPostgresTestSupport> {
|
||||
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<EmbeddedPostgres
|
|||
export async function startEmbeddedPostgresTestDatabase(
|
||||
tempDirPrefix: string,
|
||||
): Promise<EmbeddedPostgresTestDatabase> {
|
||||
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
|
||||
}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue