Skip to content

Commit 233fa6c

Browse files
authored
fix: repair pagination_cursors composite PK and isolate test suites (#265)
## What Two fixes found during post-merge testing of #262: ### CLI-72: `pagination_cursors` wrong primary key Migration 4→5 used `CREATE TABLE IF NOT EXISTS`, so any DB that already had `pagination_cursors` from an earlier code path kept a single-column PK (`command_key TEXT PRIMARY KEY`) instead of the required composite PK (`PRIMARY KEY (command_key, context)`). This caused a runtime crash on the first `--cursor last` use: ``` SQLiteError: ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint ``` **Fix:** - Schema version bumped to 6 with a migration that detects the wrong PK, drops the table, and recreates it correctly. Cursor data loss is safe (5-min TTL). - `repairWrongPrimaryKeys()` added to `repairSchema()` so auto-repair also catches this. - `isSchemaError()` extended to recognise the ON CONFLICT error and trigger auto-repair. - New `wrong_primary_key` `SchemaIssue` variant surfaced in `getSchemaIssues()` and `sentry cli fix`. ### Test isolation: `bun test` caused ~130 spurious failures locally `"test": "bun test"` ran all files in one process. `test/isolated/resolve-target.test.ts` uses `mock.module()` which leaks global module state across files, poisoning `api-client.js`, `db/defaults.js`, and others for all subsequent tests. **Fix:** Change the `test` script to `bun run test:unit && bun run test:isolated` so each suite runs in a separate Bun process. The split was already documented in `AGENTS.md`; the top-level script just wasn't using it. ## Result `bun run test`: 1931 tests, 0 failures (was 130 failures).
1 parent 995fd40 commit 233fa6c

File tree

5 files changed

+323
-19
lines changed

5 files changed

+323
-19
lines changed

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
"typecheck": "tsc --noEmit",
1818
"lint": "bunx ultracite check",
1919
"lint:fix": "bunx ultracite fix",
20-
"test": "bun test",
20+
"test": "bun run test:unit && bun run test:isolated",
2121
"test:unit": "bun test test/lib test/commands test/types --coverage --coverage-reporter=lcov",
2222
"test:isolated": "bun test test/isolated",
2323
"test:e2e": "bun test test/e2e",

src/commands/cli/fix.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ function formatIssue(issue: SchemaIssue): string {
2424
if (issue.type === "missing_table") {
2525
return `Missing table: ${issue.table}`;
2626
}
27-
return `Missing column: ${issue.table}.${issue.column}`;
27+
if (issue.type === "missing_column") {
28+
return `Missing column: ${issue.table}.${issue.column}`;
29+
}
30+
return `Wrong primary key: ${issue.table}`;
2831
}
2932

3033
/** Expected permissions for the config directory (owner rwx) */

src/lib/db/schema.ts

Lines changed: 110 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
import type { Database } from "bun:sqlite";
1515
import { stringifyUnknown } from "../errors.js";
1616

17-
export const CURRENT_SCHEMA_VERSION = 5;
17+
export const CURRENT_SCHEMA_VERSION = 6;
1818

1919
/** Environment variable to disable auto-repair */
2020
const NO_AUTO_REPAIR_ENV = "SENTRY_CLI_NO_AUTO_REPAIR";
@@ -330,6 +330,31 @@ export function hasColumn(
330330
return result.count > 0;
331331
}
332332

333+
/**
334+
* Check if a table has the expected composite primary key.
335+
*
336+
* Inspects the CREATE TABLE DDL stored in sqlite_master to verify
337+
* the table has a table-level PRIMARY KEY constraint matching the
338+
* expected columns. Returns false if the table uses per-column
339+
* PRIMARY KEY instead (e.g., `command_key TEXT PRIMARY KEY`).
340+
*/
341+
function hasCompositePrimaryKey(
342+
db: Database,
343+
table: string,
344+
expectedColumns: string[]
345+
): boolean {
346+
const row = db
347+
.query("SELECT sql FROM sqlite_master WHERE type='table' AND name=?")
348+
.get(table) as { sql: string } | undefined;
349+
350+
if (!row) {
351+
return false;
352+
}
353+
354+
const expectedPK = `PRIMARY KEY (${expectedColumns.join(", ")})`;
355+
return row.sql.includes(expectedPK);
356+
}
357+
333358
/** Add a column to a table if it doesn't exist */
334359
function addColumnIfMissing(
335360
db: Database,
@@ -345,7 +370,33 @@ function addColumnIfMissing(
345370
/** Schema issue types for diagnostics */
346371
export type SchemaIssue =
347372
| { type: "missing_table"; table: string }
348-
| { type: "missing_column"; table: string; column: string };
373+
| { type: "missing_column"; table: string; column: string }
374+
| { type: "wrong_primary_key"; table: string };
375+
376+
function findMissingColumns(db: Database, tableName: string): SchemaIssue[] {
377+
const columns = EXPECTED_COLUMNS[tableName];
378+
if (!columns) {
379+
return [];
380+
}
381+
return columns
382+
.filter((col) => !hasColumn(db, tableName, col.name))
383+
.map((col) => ({
384+
type: "missing_column" as const,
385+
table: tableName,
386+
column: col.name,
387+
}));
388+
}
389+
390+
function findPrimaryKeyIssues(db: Database, tableName: string): SchemaIssue[] {
391+
const schema = TABLE_SCHEMAS[tableName];
392+
if (
393+
schema?.compositePrimaryKey &&
394+
!hasCompositePrimaryKey(db, tableName, schema.compositePrimaryKey)
395+
) {
396+
return [{ type: "wrong_primary_key", table: tableName }];
397+
}
398+
return [];
399+
}
349400

350401
/**
351402
* Check schema and return list of issues.
@@ -360,18 +411,8 @@ export function getSchemaIssues(db: Database): SchemaIssue[] {
360411
continue;
361412
}
362413

363-
const columns = EXPECTED_COLUMNS[tableName];
364-
if (columns) {
365-
for (const col of columns) {
366-
if (!hasColumn(db, tableName, col.name)) {
367-
issues.push({
368-
type: "missing_column",
369-
table: tableName,
370-
column: col.name,
371-
});
372-
}
373-
}
374-
}
414+
issues.push(...findMissingColumns(db, tableName));
415+
issues.push(...findPrimaryKeyIssues(db, tableName));
375416
}
376417

377418
return issues;
@@ -421,8 +462,42 @@ function repairMissingColumns(db: Database, result: RepairResult): void {
421462
}
422463

423464
/**
424-
* Repair schema issues by creating missing tables and adding missing columns.
425-
* This is a non-destructive operation that only adds missing schema elements.
465+
* Drop and recreate tables that have incorrect primary key constraints.
466+
*
467+
* This fixes the CLI-72 bug where pagination_cursors was created with a
468+
* single-column PK (`command_key TEXT PRIMARY KEY`) instead of the expected
469+
* composite PK (`PRIMARY KEY (command_key, context)`). SQLite does not
470+
* support ALTER TABLE to change primary keys, so the table must be dropped
471+
* and recreated. The data loss is acceptable since pagination cursors are
472+
* ephemeral (5-minute TTL).
473+
*/
474+
function repairWrongPrimaryKeys(db: Database, result: RepairResult): void {
475+
for (const [tableName, schema] of Object.entries(TABLE_SCHEMAS)) {
476+
if (!schema.compositePrimaryKey) {
477+
continue;
478+
}
479+
if (!tableExists(db, tableName)) {
480+
continue;
481+
}
482+
if (hasCompositePrimaryKey(db, tableName, schema.compositePrimaryKey)) {
483+
continue;
484+
}
485+
try {
486+
db.exec(`DROP TABLE ${tableName}`);
487+
db.exec(EXPECTED_TABLES[tableName] as string);
488+
result.fixed.push(
489+
`Recreated table ${tableName} with correct primary key`
490+
);
491+
} catch (e) {
492+
const msg = stringifyUnknown(e);
493+
result.failed.push(`Failed to recreate table ${tableName}: ${msg}`);
494+
}
495+
}
496+
}
497+
498+
/**
499+
* Repair schema issues by creating missing tables, adding missing columns,
500+
* and recreating tables with incorrect primary key constraints.
426501
*
427502
* @param db - The raw database connection (not the traced wrapper)
428503
* @returns Lists of fixed and failed repairs
@@ -432,6 +507,7 @@ export function repairSchema(db: Database): RepairResult {
432507

433508
repairMissingTables(db, result);
434509
repairMissingColumns(db, result);
510+
repairWrongPrimaryKeys(db, result);
435511

436512
if (result.fixed.length > 0) {
437513
try {
@@ -458,7 +534,8 @@ function isSchemaError(error: unknown): boolean {
458534
return (
459535
msg.includes("no such column") ||
460536
msg.includes("no such table") ||
461-
msg.includes("has no column named")
537+
msg.includes("has no column named") ||
538+
msg.includes("on conflict clause does not match")
462539
);
463540
}
464541
return false;
@@ -615,6 +692,22 @@ export function runMigrations(db: Database): void {
615692
db.exec(EXPECTED_TABLES.pagination_cursors as string);
616693
}
617694

695+
// Migration 5 -> 6: Repair pagination_cursors if created with wrong PK (CLI-72)
696+
// Earlier versions could create the table with a single-column PK instead of
697+
// the composite PK (command_key, context). DROP + CREATE is safe because
698+
// pagination cursors are ephemeral (5-minute TTL).
699+
if (
700+
currentVersion < 6 &&
701+
tableExists(db, "pagination_cursors") &&
702+
!hasCompositePrimaryKey(db, "pagination_cursors", [
703+
"command_key",
704+
"context",
705+
])
706+
) {
707+
db.exec("DROP TABLE pagination_cursors");
708+
db.exec(EXPECTED_TABLES.pagination_cursors as string);
709+
}
710+
618711
if (currentVersion < CURRENT_SCHEMA_VERSION) {
619712
db.query("UPDATE schema_version SET version = ?").run(
620713
CURRENT_SCHEMA_VERSION

test/commands/cli/fix.test.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,4 +367,59 @@ describe("sentry cli fix", () => {
367367
// Should NOT print "Could not open database" since permission issues explain it
368368
expect(stderr).not.toContain("Could not open database");
369369
});
370+
371+
test("detects and repairs wrong primary key on pagination_cursors (CLI-72)", async () => {
372+
const dbPath = join(getTestDir(), "cli.db");
373+
const db = new Database(dbPath);
374+
// Create a full schema but with the buggy pagination_cursors table
375+
initSchema(db);
376+
db.exec("DROP TABLE pagination_cursors");
377+
db.exec(
378+
"CREATE TABLE pagination_cursors (command_key TEXT PRIMARY KEY, context TEXT NOT NULL, cursor TEXT NOT NULL, expires_at INTEGER NOT NULL)"
379+
);
380+
db.close();
381+
chmodSync(dbPath, 0o600);
382+
383+
// Warm the DB cache so getRawDatabase() uses this pre-repaired DB
384+
getDatabase();
385+
386+
const { stdout, exitCode } = await runFix(false);
387+
388+
expect(stdout).toContain("schema issue(s)");
389+
expect(stdout).toContain("Wrong primary key");
390+
expect(stdout).toContain("pagination_cursors");
391+
expect(stdout).toContain("Repairing schema");
392+
expect(stdout).toContain("repaired successfully");
393+
expect(exitCode).toBe(0);
394+
});
395+
396+
test("dry-run detects wrong primary key without repairing", async () => {
397+
const dbPath = join(getTestDir(), "cli.db");
398+
const db = new Database(dbPath);
399+
initSchema(db);
400+
db.exec("DROP TABLE pagination_cursors");
401+
db.exec(
402+
"CREATE TABLE pagination_cursors (command_key TEXT PRIMARY KEY, context TEXT NOT NULL, cursor TEXT NOT NULL, expires_at INTEGER NOT NULL)"
403+
);
404+
db.close();
405+
chmodSync(dbPath, 0o600);
406+
407+
getDatabase();
408+
409+
const { stdout } = await runFix(true);
410+
411+
expect(stdout).toContain("Wrong primary key");
412+
expect(stdout).toContain("pagination_cursors");
413+
expect(stdout).toContain("Run 'sentry cli fix' to apply fixes");
414+
// Table should still have the wrong PK
415+
closeDatabase();
416+
const verifyDb = new Database(dbPath);
417+
const row = verifyDb
418+
.query(
419+
"SELECT sql FROM sqlite_master WHERE type='table' AND name='pagination_cursors'"
420+
)
421+
.get() as { sql: string };
422+
expect(row.sql).not.toContain("PRIMARY KEY (command_key, context)");
423+
verifyDb.close();
424+
});
370425
});

0 commit comments

Comments
 (0)