Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion internal-packages/testcontainers/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
"dependencies": {
"@clickhouse/client": "^1.11.1",
"@trigger.dev/database": "workspace:*",
"ioredis": "~5.6.0"
"ioredis": "~5.6.0",
"pg": "8.15.6"
},
"devDependencies": {
"@internal/run-ops-database": "workspace:*",
"@prisma/adapter-pg": "6.14.0",
"@testcontainers/postgresql": "^11.14.0",
"@testcontainers/redis": "^11.14.0",
"@types/pg": "8.11.14",
"std-env": "^3.9.0",
"testcontainers": "^11.14.0",
"tinyexec": "^0.3.0"
Expand Down
177 changes: 177 additions & 0 deletions internal-packages/testcontainers/src/dbBlip.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
import { describe, expect } from "vitest";
import { Pool } from "pg";
import { PrismaPg } from "@prisma/adapter-pg";
import { PrismaClient } from "@trigger.dev/database";
import { postgresBlipTest } from "./index";

// A minimal infra retry, standing in for the shared read-retry util so this

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

'withRetry' is a local reimplementation of a shared retry utility, with a comment admitting it is a stand-in.

Impact: 'withRetry' is a local reimplementation of a shared retry utility, with a comment admitting it is a stand-in. This duplicates behavior and will drift from the real utility; a new hire cannot tell whether the retry semantics here match production.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

// file can demonstrate the harness end-to-end on its own.
async function withRetry<T>(fn: () => Promise<T>, maxAttempts = 8): Promise<T> {
let lastError: unknown;
for (let attempt = 0; attempt < maxAttempts; attempt++) {
try {
return await fn();
} catch (error) {
lastError = error;
await new Promise((r) => setTimeout(r, Math.min(50 * (attempt + 1), 250)));
}
}
throw lastError;
}

// Production runs the pg driver adapter, so the client under test is adapter-backed.
async function adapterClient(connectionString: string) {
const pool = new Pool({ connectionString });
// A severed idle connection makes the pg Pool emit 'error'; swallow it so an
// unhandled event can't crash the test worker before recovery is asserted.
pool.on("error", () => {});
const client = new PrismaClient({ adapter: new PrismaPg(pool) });
const dispose = async () => {
try {
await client.$disconnect();
} finally {
await pool.end();
}
};
return { client, dispose };
}

async function createProbeTable(client: PrismaClient) {
await client.$executeRawUnsafe(
`CREATE TABLE IF NOT EXISTS blip_probe (id uuid PRIMARY KEY, tag text NOT NULL)`
);
}

async function countTag(client: PrismaClient, tag: string): Promise<number> {
const rows = await client.$queryRawUnsafe<{ n: number }[]>(
`SELECT count(*)::int AS n FROM blip_probe WHERE tag = $1`,
tag
);
return rows[0]?.n ?? 0;
}

describe("DbBlipController", () => {
postgresBlipTest(
"a pooled adapter client transparently survives an idle-connection drop",
{ timeout: 60_000 },
async ({ postgresContainer, blip }) => {
const { client, dispose } = await adapterClient(postgresContainer.getConnectionUri());
try {
await client.user.count(); // warm the pool
const terminated = await blip.severIdle();
expect(terminated).toBeGreaterThan(0);
// The pool evicts the dead idle connection; the next read just works.
await new Promise((r) => setTimeout(r, 200));
const count = await client.user.count();
expect(typeof count).toBe("number");
} finally {
await dispose();
}
}
);

postgresBlipTest(
"severDuringNextStatement fails an in-flight statement",
{ timeout: 60_000 },
async ({ postgresContainer, blip }) => {
const { client, dispose } = await adapterClient(postgresContainer.getConnectionUri());
try {
const slow = client.$queryRawUnsafe(`SELECT pg_sleep(3)`);
// PrismaPromise is lazy — form the assertion so the query actually starts.
const rejected = expect(slow).rejects.toThrow();
await blip.severDuringNextStatement({ queryContains: "pg_sleep" });
await rejected;
} finally {
await dispose();
}
}
);

postgresBlipTest(
"a read recovers after a mid-flight blip",
{ timeout: 60_000 },
async ({ postgresContainer, blip }) => {
const { client, dispose } = await adapterClient(postgresContainer.getConnectionUri());
try {
const severed = client.$queryRawUnsafe(`SELECT pg_sleep(3)`).catch(() => undefined);
await blip.severDuringNextStatement({ queryContains: "pg_sleep" });
await severed;
const count = await withRetry(() => client.user.count());
expect(typeof count).toBe("number");
} finally {
await dispose();
}
}
);

postgresBlipTest(
"a non-idempotent write double-applies on retry after a post-commit blip; the idempotent form does not",
{ timeout: 60_000 },
async ({ postgresContainer, blip }) => {
const { client, dispose } = await adapterClient(postgresContainer.getConnectionUri());
try {
await createProbeTable(client);

// Model the dangerous case: the write commits, then a later statement in
// the same op is severed mid-flight (ack lost), and the caller retries.
let nonIdempotentAttempts = 0;
const nonIdempotentWrite = async () => {
nonIdempotentAttempts++;
await client.$executeRawUnsafe(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The non-idempotent write test asserts 'countTag(...) === 2', which depends on the retry actually re-executing the INSERT after the severed 'pg_sleep'.

Impact: The non-idempotent write test asserts 'countTag(...) === 2', which depends on the retry actually re-executing the INSERT after the severed 'pg_sleep'. If the sever lands before the INSERT commits, the retry produces 1 row and the test fails; if the sever lands after the sleep completes, the first attempt succeeds and the test also fails. This makes the test timing-dependent and likely flaky in CI.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

`INSERT INTO blip_probe (id, tag) VALUES (gen_random_uuid(), 'non-idempotent')`
);
if (nonIdempotentAttempts === 1) {
await client.$queryRawUnsafe(`SELECT pg_sleep(3)`); // severed → throws after the commit
}
};
const nonIdempotentDone = withRetry(nonIdempotentWrite);
await blip.severDuringNextStatement({ queryContains: "pg_sleep" });
await nonIdempotentDone;
expect(await countTag(client, "non-idempotent")).toBe(2); // the hazard, proven

// The idempotent form: a fixed id + ON CONFLICT makes the replay a no-op.
let idempotentAttempts = 0;
const idempotentWrite = async () => {
idempotentAttempts++;
await client.$executeRawUnsafe(
`INSERT INTO blip_probe (id, tag)
VALUES ('00000000-0000-0000-0000-000000000001', 'idempotent')
ON CONFLICT (id) DO NOTHING`
);
if (idempotentAttempts === 1) {
await client.$queryRawUnsafe(`SELECT pg_sleep(3)`);
}
};
const idempotentDone = withRetry(idempotentWrite);
await blip.severDuringNextStatement({ queryContains: "pg_sleep" });
await idempotentDone;
expect(await countTag(client, "idempotent")).toBe(1); // exactly once despite retry
} finally {
await dispose();
}
}
);

// Regression: queryContains must match as literal text, not as an ILIKE pattern.
// The active query contains "fooXbar"; under ILIKE the pattern "foo_bar" (with the
// wildcard `_`) would wrongly match and terminate it. The literal matcher must not,
// so the sever times out instead of killing the wrong statement.
postgresBlipTest(
"severDuringNextStatement matches queryContains literally, not as an ILIKE pattern",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The literal-match regression test is racy: the slow query is started before the sever call, but the test only awaits the sever rejection and then disposes the client before awaitin

Impact: The literal-match regression test is racy: the slow query is started before the sever call, but the test only awaits the sever rejection and then disposes the client before awaiting 'slow'. If the query is still running when 'dispose()' runs, 'pool.end()' may hang or the catch may never settle, causing a flaky timeout or unhandled rejection.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

{ timeout: 60_000 },
async ({ postgresContainer, blip }) => {
const { client, dispose } = await adapterClient(postgresContainer.getConnectionUri());
const slow = client
.$queryRawUnsafe(`SELECT pg_sleep(3) /* marker fooXbar */`)
.catch(() => undefined);
try {
await expect(
blip.severDuringNextStatement({ queryContains: "foo_bar", timeoutMs: 1000, pollMs: 25 })
).rejects.toThrow(/no active statement/i);
} finally {
await dispose();
await slow;
}
}
);
});
107 changes: 107 additions & 0 deletions internal-packages/testcontainers/src/dbBlip.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import { Client } from "pg";

/**
* Simulates a connection blip against a test Postgres (via a separate admin
* connection that terminates backends), so a vertical can prove its DB code
* survives a disconnect. Reproduces the mid-statement / stale-connection
* signatures (P1017, "Connection terminated unexpectedly").
*/
export type DbBlipController = {
/** Terminate every idle client backend except this harness's own, so the
* next operation hits a dead connection. Returns the number terminated. */
severIdle(): Promise<number>;

/** Poll for an active client statement (optionally matching `queryContains`
* literally), then terminate it mid-flight. Rejects if none appears within
* `timeoutMs`. Terminating by pid isn't atomic with statement completion, so
* target a statement with a real execution window (e.g. `pg_sleep`) — a query
* that finishes first leaves its connection idle and it is closed anyway. */
severDuringNextStatement(opts?: {
queryContains?: string;
timeoutMs?: number;
pollMs?: number;
}): Promise<void>;
};

/** A {@link DbBlipController} plus the teardown for its admin connection. */
export type DbBlipHandle = DbBlipController & { close(): Promise<void> };

// Reserved application_name for the harness's control connections. The severs
// exclude every connection using it (by name, plus their own pid), so multiple
// controllers on one database can't kill each other's admin. A client-under-test
// must not use this name.
const ADMIN_APPLICATION_NAME = "trigger-db-blip-admin";

const sleep = (ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms));

/** Opens an isolated admin connection and returns a handle that can sever the
* other connections on that database. `close()` in teardown. */
export async function createDbBlipController(connectionUri: string): Promise<DbBlipHandle> {
// Raw pg (not Prisma): the control connection must be one identifiable backend we can exclude from the sever, independent of the client under test.
const admin = new Client({
connectionString: connectionUri,
application_name: ADMIN_APPLICATION_NAME,
});
await admin.connect();
// Swallow async connection errors so a consumer that severs a DB the admin
// isn't excluded from (or drops it while open) can't crash the test worker.
admin.on("error", () => {});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The admin connection swallows all 'error' events with an empty handler.

Impact: The admin connection swallows all 'error' events with an empty handler. This can hide authentication failures, connection drops, or protocol errors during setup, causing the harness to fail later with misleading 'no active statement' errors instead of surfacing the real cause.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


async function severIdle(): Promise<number> {
const result = await admin.query<{ terminated: boolean }>(
`SELECT pg_terminate_backend(pid) AS terminated
FROM pg_stat_activity
WHERE datname = current_database()
AND pid <> pg_backend_pid()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

'severIdle' and 'severDuringNextStatement' use 'pg_terminate_backend' against all client backends except the admin connection.

Impact: 'severIdle' and 'severDuringNextStatement' use 'pg_terminate_backend' against all client backends except the admin connection. If a test database is shared or a non-test client connects with a different 'application_name', this harness will terminate unrelated backends, potentially disrupting other tests or local development sessions.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

AND backend_type = 'client backend'
AND application_name IS DISTINCT FROM $1
AND state = 'idle'`,
[ADMIN_APPLICATION_NAME]
);
// Count only backends that were actually terminated (a backend that exits
// between selection and signalling returns false).
return result.rows.filter((row) => row.terminated === true).length;
}

async function severDuringNextStatement(opts?: {
queryContains?: string;
timeoutMs?: number;
pollMs?: number;
}): Promise<void> {
const queryContains = opts?.queryContains ?? null;
const timeoutMs = opts?.timeoutMs ?? 5000;
const pollMs = opts?.pollMs ?? 25;
const deadline = Date.now() + timeoutMs;

while (Date.now() < deadline) {
// Select and terminate in one statement so the backend can't go idle
// between picking it and killing it; return only when it was terminated.
const terminated = await admin.query<{ ok: boolean }>(
`SELECT pg_terminate_backend(pid) AS ok
FROM pg_stat_activity
WHERE datname = current_database()
AND state = 'active'
AND pid <> pg_backend_pid()
AND backend_type = 'client backend'
AND application_name IS DISTINCT FROM $1
AND ($2::text IS NULL OR strpos(lower(query), lower($2)) > 0)
LIMIT 1`,
[ADMIN_APPLICATION_NAME, queryContains]
);

if (terminated.rows[0]?.ok === true) {
return;
}

await sleep(pollMs);
}

throw new Error(
`severDuringNextStatement: no active statement${
queryContains ? ` matching ${JSON.stringify(queryContains)}` : ""
} appeared within ${timeoutMs}ms`
);
}

return { severIdle, severDuringNextStatement, close: () => admin.end() };
}
28 changes: 28 additions & 0 deletions internal-packages/testcontainers/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
runClickhouseMigrations,
truncateClickhouseTables,
} from "./clickhouse";
import { createDbBlipController, type DbBlipController } from "./dbBlip";
import { getTaskMetadata, logCleanup, logSetup } from "./logs";
import { type MinIOConnectionConfig, type StartedMinIOContainer, MinIOContainer } from "./minio";
import {
Expand All @@ -35,6 +36,7 @@ export {
} from "./utils";
export { OtelCollectorContainer, StartedOtelCollectorContainer } from "./otelCollector";
export { laggingReplica, type LaggingModel } from "./laggingReplica";
export { createDbBlipController, type DbBlipController, type DbBlipHandle } from "./dbBlip";
export { logCleanup };
export type { MinIOConnectionConfig };

Expand Down Expand Up @@ -353,6 +355,32 @@ export const postgresTest = withWarmup(
}
);

export type PostgresBlipTestContext = PostgresTestContext & { blip: DbBlipController };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The 'blipFromContainer' helper is exported indirectly through 'postgresBlipTest' but its type signature mixes 'StartedPostgreSqlContainer' and 'TestContext' in a way that obscures

Impact: The 'blipFromContainer' helper is exported indirectly through 'postgresBlipTest' but its type signature mixes 'StartedPostgreSqlContainer' and 'TestContext' in a way that obscures which fields are actually available. The 'TestContext' intersection appears unused and will confuse readers about the contract.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


const blipFromContainer = async (
{ postgresContainer }: { postgresContainer: StartedPostgreSqlContainer } & TestContext,
use: Use<DbBlipController>
) => {
const handle = await createDbBlipController(postgresContainer.getConnectionUri());
try {
await use(handle);
} finally {
await handle.close();
}
};

// postgresTest + a DbBlipController bound to the same per-test database.
export const postgresBlipTest = withWarmup(
test.extend<PostgresBlipTestContext>({
postgresContainer: clonedPostgresContainer,
prisma: prismaFromContainer,
blip: blipFromContainer,
}),
async () => {
await getWorkerPostgresContainer();
}
);

type HeteroPostgresTestContext = {
// PG14 (legacy / control-plane DB analog)
postgresContainer14: StartedPostgreSqlContainer;
Expand Down
9 changes: 9 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading