Files
Ekklesia/core/tests/postgres-migration-database.test.ts
T
ekklesiaandClaude Opus 5 2983789779 fix: harden migration command execution
Two problems in the migration commands: no timeouts, and SQL split on literal
bytes.

`migrations:plan` and `migrations:apply` constructed their connection with
neither a connection deadline nor an operation deadline — a real server reported
`statement_timeout = 0` on the migration connection. Both now default to 10
seconds for the connection deadline, the client query timeout, and PostgreSQL's
own statement_timeout, applied at the one shared construction boundary so a
future call site cannot silently omit them. History reads, advisory-lock
queries, transaction control, migration SQL, and history writes are all covered.
The existing lock-wait deadline, the Pass 2 TLS policy, and Doctor's and the
bootstrap CLI's own timeouts are untouched, and no environment variable or flag
was added.

Explicit client fields do not win under the pinned driver. [email protected].0 builds
parameters as `Object.assign({}, config, parse(config.connectionString))`, so
the URL overrides passed options — verified directly, where an explicit
statement_timeout of 10000 alongside `?statement_timeout=0` produced a
server-reported 0, while a clean URL produced 10s. A DATABASE_URL carrying
connect_timeout, query_timeout, statement_timeout, or an `options` value setting
statement_timeout is therefore refused with the existing fixed configuration
error, before any client is constructed and without echoing the URL, parameter,
or value. Unrelated parameters are left alone.

The runner split each migration on the literal `--> statement-breakpoint` bytes.
Those bytes inside a string literal, a dollar-quoted function body, a quoted
identifier, or a comment tore the migration into fragments that were no longer
valid SQL — each case verified to produce two fragments where one was required.
Each migration's exact SQL is now passed to the client once, with no values and
no prepared-statement name, inside the existing transaction, and its history row
inserted only after that batch succeeds. One transaction still spans all pending
migrations; advisory locking, checksum validation, approval preflight, ordering,
rollback, cleanup, and sanitized error identity are unchanged. The port method
is renamed executeStatement to executeMigrationSql so the contract says what it
receives.

PostgreSQL parses the batch because it owns the only correct parser. A homegrown
splitter would have to track single quotes, dollar quoting with arbitrary tags,
quoted identifiers, line comments, nested block comments, and escape syntax to
answer a question the server already answers, and a parser dependency would add
supply-chain surface for the same reason. Drizzle's marker lines are ordinary
`--` comments when the batch is sent whole. No custom SQL parser exists here.

Co-Authored-By: Claude Opus 5 <[email protected]>
2026-07-26 23:13:52 -05:00

222 lines
8.0 KiB
TypeScript

import { describe, expect, it, vi } from "vitest";
const clientConstructor = vi.hoisted(() => vi.fn());
vi.mock("pg", () => ({ Client: clientConstructor }));
import {
createPostgresMigrationDatabase,
MIGRATION_CONNECTION_TIMEOUT_MS,
MIGRATION_OPERATION_TIMEOUT_MS,
PostgresMigrationDatabaseError,
} from "../src/migrations/postgres-database.js";
describe("PostgreSQL migration database composition", () => {
it("does not construct a client or execute SQL merely because the adapter is imported", () => {
expect(clientConstructor).not.toHaveBeenCalled();
});
it.each([
undefined,
"",
"not-a-database-url",
"https://admin:[email protected]/database",
])("rejects missing or invalid configuration without exposing it (%s)", (databaseUrl) => {
let captured: unknown;
try {
createPostgresMigrationDatabase(databaseUrl);
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
const publicText = `${String(captured)}\n${
captured instanceof Error ? (captured.stack ?? "") : ""
}\n${JSON.stringify(captured)}`;
if (databaseUrl) {
expect(publicText).not.toContain(databaseUrl);
}
expect(clientConstructor).not.toHaveBeenCalled();
});
it("creates a lazy database composition without opening a connection", () => {
const database = createPostgresMigrationDatabase(
"postgresql://operator:[email protected]/ekklesia",
);
expect(Object.isFrozen(database)).toBe(true);
expect(clientConstructor).not.toHaveBeenCalled();
expect(JSON.stringify(database)).not.toContain("operator");
expect(JSON.stringify(database)).not.toContain("do-not-log");
expect(JSON.stringify(database)).not.toContain("example.test");
});
it.each([
["connectionTimeoutMs", 0],
["connectionTimeoutMs", -1],
["connectionTimeoutMs", 1.5],
["connectionTimeoutMs", Number.NaN],
["connectionTimeoutMs", Number.POSITIVE_INFINITY],
["connectionTimeoutMs", 2_147_483_648],
["operationTimeoutMs", 0],
["operationTimeoutMs", -1],
["operationTimeoutMs", 1.5],
["operationTimeoutMs", Number.NaN],
["operationTimeoutMs", Number.POSITIVE_INFINITY],
["operationTimeoutMs", 2_147_483_648],
] as const)("rejects an invalid %s value without constructing a client", (name, value) => {
let captured: unknown;
try {
createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
[name]: value,
});
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
expect(clientConstructor).not.toHaveBeenCalled();
});
it("passes bounded connection and operation timeouts to the PostgreSQL client", async () => {
const client = {
connect: vi.fn().mockResolvedValue(undefined),
end: vi.fn().mockResolvedValue(undefined),
on: vi.fn(),
};
const MockClient = class {
readonly connect = client.connect;
readonly end = client.end;
readonly on = client.on;
};
clientConstructor.mockImplementation(MockClient as unknown as () => typeof client);
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
connectionTimeoutMs: 10_000,
operationTimeoutMs: 12_000,
});
const session = await database.openSession();
expect(clientConstructor).toHaveBeenCalledWith({
connectionString: "postgresql://example.test/ekklesia",
connectionTimeoutMillis: 10_000,
query_timeout: 12_000,
statement_timeout: 12_000,
});
expect(client.on).toHaveBeenCalledWith("error", expect.any(Function));
expect(client.connect).toHaveBeenCalledOnce();
await session.close();
expect(client.end).toHaveBeenCalledOnce();
});
});
/**
* Pass 4: finite migration timeouts live at this one construction boundary, and the pinned driver's
* connection-string precedence cannot weaken them.
*/
describe("PostgreSQL migration timeout policy", () => {
function mockClient() {
const client = {
connect: vi.fn().mockResolvedValue(undefined),
end: vi.fn().mockResolvedValue(undefined),
on: vi.fn(),
};
clientConstructor.mockImplementation(
class {
readonly connect = client.connect;
readonly end = client.end;
readonly on = client.on;
} as unknown as () => typeof client,
);
return client;
}
it("applies the fixed 10-second defaults when a caller omits them", async () => {
mockClient();
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia");
const session = await database.openSession();
// This is exactly what `migrations:plan` and `migrations:apply` construct: no timeout options.
expect(clientConstructor).toHaveBeenCalledWith({
connectionString: "postgresql://example.test/ekklesia",
connectionTimeoutMillis: MIGRATION_CONNECTION_TIMEOUT_MS,
query_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
});
expect(MIGRATION_CONNECTION_TIMEOUT_MS).toBe(10_000);
expect(MIGRATION_OPERATION_TIMEOUT_MS).toBe(10_000);
await session.close();
});
it("still honours an explicit narrower test override", async () => {
mockClient();
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
connectionTimeoutMs: 250,
operationTimeoutMs: 500,
});
const session = await database.openSession();
expect(clientConstructor).toHaveBeenCalledWith({
connectionString: "postgresql://example.test/ekklesia",
connectionTimeoutMillis: 250,
query_timeout: 500,
statement_timeout: 500,
});
await session.close();
});
it.each([
"postgresql://example.test/ekklesia?statement_timeout=0",
"postgresql://example.test/ekklesia?STATEMENT_TIMEOUT=0",
"postgresql://example.test/ekklesia?query_timeout=0",
"postgresql://example.test/ekklesia?connect_timeout=0",
"postgresql://example.test/ekklesia?options=-c%20statement_timeout%3D0",
"postgresql://example.test/ekklesia?statement_timeout=900000",
])("refuses a connection string that would override the policy (%s)", (databaseUrl) => {
// The pinned [email protected] merges as `Object.assign({}, config, parse(connectionString))`, so the
// URL wins over the explicit values above — verified against the real driver, where an explicit
// `statement_timeout: 10000` alongside `?statement_timeout=0` produced a server-reported `0`.
const constructionsBefore = clientConstructor.mock.calls.length;
let captured: unknown;
try {
createPostgresMigrationDatabase(databaseUrl);
} catch (error) {
captured = error;
}
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
// The rejection never echoes the URL, the offending parameter, or its value.
const publicText = `${String(captured)}\n${
captured instanceof Error ? (captured.stack ?? "") : ""
}`;
expect(publicText).not.toContain("example.test");
expect(publicText).not.toContain("statement_timeout=");
expect(publicText).not.toContain("900000");
// Rejected before any client is constructed, so no connection is ever attempted.
expect(clientConstructor.mock.calls.length).toBe(constructionsBefore);
});
it("leaves unrelated connection-string parameters alone", async () => {
mockClient();
const database = createPostgresMigrationDatabase(
"postgresql://example.test/ekklesia?application_name=ekklesia&lock_timeout=5000",
);
const session = await database.openSession();
expect(clientConstructor).toHaveBeenCalledWith(
expect.objectContaining({ statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS }),
);
await session.close();
});
});