mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 16:27:18 +02:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The `packages/db` module owns all database migrations via a sequential numbering system already validated at build time > - A recent migration introduced an O(n²) batch backfill over a large, unindexed table — it caused the server's `listen` to block for ~5 minutes on databases with millions of rows > - Nothing in the current CI pipeline catches large-table migration risk patterns (DO-loop mutations, batched LIMIT mutations without support indexes, full-table mutations, non-concurrent index creation) before they land > - This PR wires a new static migration-safety checker (`check-migration-safety.ts`) into the existing `check:migrations` gate in `packages/db/package.json`, so risky patterns fail CI before reaching production > - The checker baselines all historical findings already present in the codebase, so the gate fails only on *new* unbaselined risky patterns > - The benefit is that the specific O(n²) backfill shape (and related patterns) will be caught at author time rather than at incident time ## Linked Issues or Issue Description **Feature — static migration safety lint** **Problem or motivation** Migrations against large tables (millions of rows) have caused production startup blocks. The root pattern is a batched `LIMIT`-based backfill iterating via an unindexed column, making each batch a sequential scan — O(n²) overall. No CI gate exists to flag this class of problem before merge. **Proposed solution** A static SQL-level checker that scans new migration files for known dangerous patterns against known-large tables, producing structured findings that are either baselined (suppressed) or fail the build. Patterns detected: `DO $$ loop` mutations on large tables without a same-migration support index, batched `LIMIT` mutations on large tables missing a same-migration support index, unbounded full-table mutations (no `WHERE` clause), and `CREATE INDEX` without `CONCURRENTLY` on large tables. **Alternatives considered** Runtime instrumentation (only catches issues in production), advisory locking in migrations (doesn't prevent the pattern), per-migration code review (doesn't scale consistently). **Roadmap alignment** Defensive infrastructure / operational reliability — keeps migrations from blocking production startups. Not a user-facing feature. ## What Changed - **`packages/db/package.json`** — extended `check:migrations` script to run `check-migration-safety.ts` after the existing numbering check - **`packages/db/src/check-migration-safety.ts`** — new static checker: SQL pattern matching, rule detection for four dangerous patterns, baseline diffing, and structured exit with findings summary - **`packages/db/src/migration-safety-baseline.ts`** — baseline of all existing historical findings (suppressed from failing the gate); new migrations matching these patterns without a baseline entry will fail - **`packages/db/src/table-size-estimates.ts`** — rough table size estimates from the local dev database; drives `isKnownLargeTable()` used by the safety rules - **`packages/db/src/check-migration-safety.test.ts`** — Vitest coverage for the O(n²) backfill failure mode, suppression via baseline, and each rule type ## Verification ```bash # Run the migration safety checker directly cd packages/db tsx src/check-migration-safety.ts # Run tests cd packages/db npx vitest run src/check-migration-safety.test.ts # Run the full migration check gate (numbering + safety) cd packages/db pnpm run check:migrations ``` - Tests cover the core O(n²) backfill pattern (the motivating incident), baseline suppression, and all four rule types - `check:migrations` now exits non-zero for any new unbaselined large-table migration risk pattern ## Risks - **False positives:** Table-size estimates are from a local dev database snapshot — a table small in dev but large in production would be missed. Best-effort heuristic. - **Baseline drift:** If a baselined finding's SQL changes significantly, the baseline ID (content-hash-based) will no longer match and the finding will re-surface. Intentional but may surprise authors doing incremental fixes. - **SQL parsing limitations:** Regex-based pattern matching rather than a full AST parser — complex SQL may not be detected. Acceptable for an initial gate. - **Low risk to existing behavior:** The gate only fails on *new* findings not present in the baseline. All existing migrations are baselined. ## Model Used - **Provider:** Anthropic - **Model ID:** `claude-sonnet-4-6` - **Context window:** 200K - **Tool use:** yes (file reading, bash, git operations) - **Reasoning mode:** standard (no extended thinking) ## 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 - [ ] 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>
493 lines
14 KiB
TypeScript
493 lines
14 KiB
TypeScript
import { describe, expect, it } from "vitest";
|
|
import {
|
|
analyzeMigrationSafety,
|
|
type MigrationSafetyInput,
|
|
} from "./check-migration-safety.js";
|
|
import {
|
|
TABLE_SIZE_ESTIMATE_FACTOR,
|
|
TABLE_SIZE_BUCKET_THRESHOLDS,
|
|
type TableSizeEstimate,
|
|
} from "./table-size-estimates.js";
|
|
|
|
const testEstimates: readonly TableSizeEstimate[] = [
|
|
{
|
|
table: "issue_comments",
|
|
localRows: 5_034,
|
|
estimateFactor: TABLE_SIZE_ESTIMATE_FACTOR,
|
|
estimatedRows: 5_034 * TABLE_SIZE_ESTIMATE_FACTOR,
|
|
bucket: "large",
|
|
},
|
|
{
|
|
table: "companies",
|
|
localRows: 1,
|
|
estimateFactor: TABLE_SIZE_ESTIMATE_FACTOR,
|
|
estimatedRows: TABLE_SIZE_ESTIMATE_FACTOR,
|
|
bucket: "small",
|
|
},
|
|
];
|
|
|
|
function analyze(sql: string) {
|
|
const migrations: readonly MigrationSafetyInput[] = [{ fileName: "9999_fixture.sql", sql }];
|
|
return analyzeMigrationSafety(migrations, { baselineIds: [], estimates: testEstimates });
|
|
}
|
|
|
|
describe("migration safety check", () => {
|
|
it("documents the large table threshold used by the estimates", () => {
|
|
expect(TABLE_SIZE_BUCKET_THRESHOLDS.largeRows).toBe(1_000_000);
|
|
expect(testEstimates[0]?.estimatedRows).toBeGreaterThanOrEqual(
|
|
TABLE_SIZE_BUCKET_THRESHOLDS.largeRows,
|
|
);
|
|
});
|
|
|
|
it("fails a 0126-shaped batched loop over a large table without a support index", () => {
|
|
const result = analyze(`
|
|
DO $$
|
|
DECLARE
|
|
last_comment_id uuid := '00000000-0000-0000-0000-000000000000'::uuid;
|
|
BEGIN
|
|
LOOP
|
|
WITH batch AS MATERIALIZED (
|
|
SELECT c."id"
|
|
FROM "issue_comments" c
|
|
WHERE c."id" > last_comment_id
|
|
AND c."author_agent_id" IS NULL
|
|
ORDER BY c."id"
|
|
LIMIT 5000
|
|
)
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM batch b
|
|
WHERE c."id" = b."id";
|
|
|
|
EXIT WHEN NOT FOUND;
|
|
END LOOP;
|
|
END $$;
|
|
`);
|
|
|
|
expect(result.newFindings.map((finding) => finding.rule)).toEqual(
|
|
expect.arrayContaining([
|
|
"loop-mutation-large-table",
|
|
"batched-mutation-large-table-missing-index",
|
|
]),
|
|
);
|
|
expect(result.newFindings[0]?.table).toBe("issue_comments");
|
|
});
|
|
|
|
it("passes the same bounded large-table backfill when a matching concurrent support index exists", () => {
|
|
const result = analyze(`
|
|
CREATE INDEX CONCURRENTLY IF NOT EXISTS "issue_comments_fixture_backfill_idx"
|
|
ON "issue_comments" USING btree ("id")
|
|
WHERE "author_agent_id" IS NULL;--> statement-breakpoint
|
|
DO $$
|
|
DECLARE
|
|
last_comment_id uuid := '00000000-0000-0000-0000-000000000000'::uuid;
|
|
BEGIN
|
|
LOOP
|
|
WITH batch AS MATERIALIZED (
|
|
SELECT c."id"
|
|
FROM "issue_comments" c
|
|
WHERE c."id" > last_comment_id
|
|
AND c."author_agent_id" IS NULL
|
|
ORDER BY c."id"
|
|
LIMIT 5000
|
|
)
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM batch b
|
|
WHERE c."id" = b."id";
|
|
|
|
EXIT WHEN NOT FOUND;
|
|
END LOOP;
|
|
END $$;
|
|
`);
|
|
|
|
expect(result.newFindings).toEqual([]);
|
|
});
|
|
|
|
it("does not suppress missing-index finding when a partial support index predicate is incompatible with the batch WHERE", () => {
|
|
const result = analyze(`
|
|
CREATE INDEX CONCURRENTLY IF NOT EXISTS "issue_comments_fixture_backfill_idx"
|
|
ON "issue_comments" USING btree ("id")
|
|
WHERE "author_agent_id" IS NOT NULL;--> statement-breakpoint
|
|
DO $$
|
|
DECLARE
|
|
last_comment_id uuid := '00000000-0000-0000-0000-000000000000'::uuid;
|
|
BEGIN
|
|
LOOP
|
|
WITH batch AS MATERIALIZED (
|
|
SELECT c."id"
|
|
FROM "issue_comments" c
|
|
WHERE c."id" > last_comment_id
|
|
AND c."author_agent_id" IS NULL
|
|
ORDER BY c."id"
|
|
LIMIT 5000
|
|
)
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM batch b
|
|
WHERE c."id" = b."id";
|
|
|
|
EXIT WHEN NOT FOUND;
|
|
END LOOP;
|
|
END $$;
|
|
`);
|
|
|
|
expect(result.newFindings.map((finding) => finding.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("passes a batched backfill over a small-bucket table", () => {
|
|
const result = analyze(`
|
|
DO $$
|
|
BEGIN
|
|
LOOP
|
|
WITH batch AS (
|
|
SELECT "id"
|
|
FROM "companies"
|
|
ORDER BY "id"
|
|
LIMIT 100
|
|
)
|
|
UPDATE "companies" c
|
|
SET "description" = c."description"
|
|
FROM batch b
|
|
WHERE c."id" = b."id";
|
|
|
|
EXIT WHEN NOT FOUND;
|
|
END LOOP;
|
|
END $$;
|
|
`);
|
|
|
|
expect(result.newFindings).toEqual([]);
|
|
});
|
|
|
|
it("flags UPDATE ... FROM (SELECT ... LIMIT N) subquery batch on a large table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "author_agent_id" IS NULL
|
|
ORDER BY "id"
|
|
LIMIT 5000
|
|
) batch
|
|
WHERE c."id" = batch."id";
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags UPDATE ... FROM (SELECT ... FETCH FIRST N ROWS ONLY) subquery batch on a large table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "author_agent_id" IS NULL
|
|
ORDER BY "id"
|
|
FETCH FIRST 5000 ROWS ONLY
|
|
) batch
|
|
WHERE c."id" = batch."id";
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags UPDATE ... FROM (SELECT ... FETCH NEXT N ROWS ONLY) subquery batch on a large table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "author_agent_id" IS NULL
|
|
ORDER BY "id"
|
|
FETCH NEXT 5000 ROWS ONLY
|
|
) batch
|
|
WHERE c."id" = batch."id";
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags UPDATE ... FROM (SELECT ... FETCH FIRST N ROWS WITH TIES) subquery batch on a large table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "author_agent_id" IS NULL
|
|
ORDER BY "id"
|
|
FETCH FIRST 5000 ROWS WITH TIES
|
|
) batch
|
|
WHERE c."id" = batch."id";
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags UPDATE ... FROM (SELECT ... FETCH FIRST ROW ONLY) subquery batch on a large table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "author_agent_id" IS NULL
|
|
ORDER BY "id"
|
|
FETCH FIRST ROW ONLY
|
|
) batch
|
|
WHERE c."id" = batch."id";
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags UPDATE ... WHERE IN (SELECT ... LIMIT N) subquery batch on a large table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL
|
|
WHERE "id" IN (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "author_agent_id" IS NULL
|
|
ORDER BY "id"
|
|
LIMIT 5000
|
|
);
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags a CTE with a selective WHERE when the outer UPDATE has no WHERE clause", () => {
|
|
const result = analyze(`
|
|
WITH selective AS (
|
|
SELECT "id" FROM "issue_comments" WHERE "author_agent_id" IS NULL
|
|
)
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM selective;
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not treat WHERE inside a block comment as a selective predicate", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL /* ignored
|
|
/* nested WHERE "id" > '0' */
|
|
still ignored
|
|
*/;
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not treat WHERE inside an inline line comment as a selective predicate", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL -- WHERE "id" > '0'
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not treat WHERE inside a string literal as a selective predicate", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "body" = 'WHERE "id" > ''0''';
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not treat WHERE inside a tagged dollar-quoted string as a selective predicate", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "body" = $msg$WHERE "id" > '0'$msg$;
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not treat WHERE inside an untagged dollar-quoted string as a selective predicate", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "body" = $$WHERE "id" > '0'$$;
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("still accepts a real selective WHERE clause", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL
|
|
WHERE "id" > '0';
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).not.toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not suppress full-table finding when WHERE only constrains a joined table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM "companies"
|
|
WHERE "companies"."id" = '00000000-0000-0000-0000-000000000000';
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not suppress full-table finding when WHERE only constrains a joined table via an unquoted alias", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM "companies" c
|
|
WHERE c."id" = '00000000-0000-0000-0000-000000000000';
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not suppress full-table finding when WHERE only constrains an unquoted joined table", () => {
|
|
const result = analyze(`
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM companies
|
|
WHERE companies.id = '00000000-0000-0000-0000-000000000000';
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"full-table-mutation-large-table",
|
|
);
|
|
});
|
|
|
|
it("does not suppress missing-index finding when support index uses an expression", () => {
|
|
const result = analyze(`
|
|
CREATE INDEX CONCURRENTLY IF NOT EXISTS "issue_comments_expr_idx"
|
|
ON "issue_comments" ((lower("body")));--> statement-breakpoint
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
ORDER BY "id"
|
|
LIMIT 5000
|
|
) b
|
|
WHERE c."id" = b."id";
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags a batch backfill when the support index does not cover the ORDER BY key", () => {
|
|
const result = analyze(`
|
|
CREATE INDEX CONCURRENTLY IF NOT EXISTS "issue_comments_author_idx"
|
|
ON "issue_comments" ("author_agent_id");--> statement-breakpoint
|
|
DO $$
|
|
DECLARE
|
|
last_id uuid := '00000000-0000-0000-0000-000000000000'::uuid;
|
|
BEGIN
|
|
LOOP
|
|
WITH batch AS MATERIALIZED (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "id" > last_id
|
|
ORDER BY "id"
|
|
LIMIT 5000
|
|
)
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM batch b
|
|
WHERE c."id" = b."id";
|
|
|
|
EXIT WHEN NOT FOUND;
|
|
END LOOP;
|
|
END $$;
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("flags a batch backfill when the support index only covers a later ORDER BY column", () => {
|
|
const result = analyze(`
|
|
CREATE INDEX CONCURRENTLY IF NOT EXISTS "issue_comments_created_idx"
|
|
ON "issue_comments" ("created_at");--> statement-breakpoint
|
|
DO $$
|
|
DECLARE
|
|
last_id uuid := '00000000-0000-0000-0000-000000000000'::uuid;
|
|
BEGIN
|
|
LOOP
|
|
WITH batch AS MATERIALIZED (
|
|
SELECT "id"
|
|
FROM "issue_comments"
|
|
WHERE "id" > last_id
|
|
ORDER BY "id", "created_at"
|
|
LIMIT 5000
|
|
)
|
|
UPDATE "issue_comments" c
|
|
SET "derived_author_agent_id" = NULL
|
|
FROM batch b
|
|
WHERE c."id" = b."id";
|
|
|
|
EXIT WHEN NOT FOUND;
|
|
END LOOP;
|
|
END $$;
|
|
`);
|
|
|
|
expect(result.newFindings.map((f) => f.rule)).toContain(
|
|
"batched-mutation-large-table-missing-index",
|
|
);
|
|
});
|
|
|
|
it("honors suppressions only when they name a rule and reason", () => {
|
|
const result = analyze(`
|
|
-- paperclip:migration-safety-ignore full-table-mutation-large-table: one-time metadata reset approved in issue thread
|
|
UPDATE "issue_comments"
|
|
SET "derived_author_source" = NULL;
|
|
`);
|
|
|
|
expect(result.newFindings).toEqual([]);
|
|
});
|
|
});
|