Skip to content

Commit 26e04a6

Browse files
committed
fix(db): make column migrations atomic
Column migrations may pair schema changes with one-time data backfills. If startup fails between those statements, detecting only column existence can suppress the repair permanently. Commit the batch atomically so failed opens leave the schema retryable.
1 parent f4c72fe commit 26e04a6

2 files changed

Lines changed: 77 additions & 6 deletions

File tree

internal/db/db.go

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2146,11 +2146,26 @@ func schemaColumnMigrations() []schemaColumnMigration {
21462146
}
21472147
}
21482148

2149-
func applySchemaColumnMigrations(
2150-
queryRow func(string, ...any) rowScanner,
2151-
exec func(string, ...any) (sql.Result, error),
2152-
) error {
2153-
return applyColumnMigrations(schemaColumnMigrations(), queryRow, exec)
2149+
func applySchemaColumnMigrations(w *writerHandle) error {
2150+
tx, err := w.BeginTx(context.Background(), nil)
2151+
if err != nil {
2152+
return fmt.Errorf("starting column migration transaction: %w", err)
2153+
}
2154+
defer func() { _ = tx.Rollback() }()
2155+
2156+
if err := applyColumnMigrations(
2157+
schemaColumnMigrations(),
2158+
func(query string, args ...any) rowScanner {
2159+
return tx.QueryRow(query, args...)
2160+
},
2161+
tx.Exec,
2162+
); err != nil {
2163+
return err
2164+
}
2165+
if err := tx.Commit(); err != nil {
2166+
return fmt.Errorf("committing column migrations: %w", err)
2167+
}
2168+
return nil
21542169
}
21552170

21562171
func applyColumnMigrations(
@@ -2396,7 +2411,7 @@ func (db *DB) migrateColumns() error {
23962411
if _, err := w.Exec(artifactSessionQueueTriggerDropsSQL); err != nil {
23972412
return fmt.Errorf("dropping artifact session queue triggers: %w", err)
23982413
}
2399-
if err := applySchemaColumnMigrations(w.QueryRow, w.Exec); err != nil {
2414+
if err := applySchemaColumnMigrations(w); err != nil {
24002415
return err
24012416
}
24022417
if _, err := w.Exec(artifactSessionQueueTriggerCreatesSQL); err != nil {

internal/db/legacy_schema_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,62 @@ func TestParserParentSessionIDMigrationBackfillsCurrentParent(t *testing.T) {
261261
assert.Equal(t, "parsed-parent", got.String, "migrated parser parent")
262262
}
263263

264+
func TestParserParentSessionIDMigrationRollsBackWhenBackfillFails(t *testing.T) {
265+
path := filepath.Join(t.TempDir(), "legacy.db")
266+
conn, err := sql.Open("sqlite3", makeDSN(path, false))
267+
require.NoError(t, err)
268+
conn.SetMaxOpenConns(1)
269+
270+
_, err = conn.Exec(v06LegacySchema)
271+
require.NoError(t, err, "create legacy schema")
272+
_, err = conn.Exec(`
273+
INSERT INTO sessions (
274+
id, project, machine, agent, parent_session_id
275+
) VALUES (
276+
'kid', 'project-a', 'local', 'claude', 'parsed-parent'
277+
);
278+
CREATE TRIGGER fail_parser_parent_backfill
279+
BEFORE UPDATE OF parser_parent_session_id ON sessions BEGIN
280+
SELECT RAISE(ABORT, 'injected parser parent backfill failure');
281+
END;`)
282+
require.NoError(t, err, "prepare failing legacy migration")
283+
_, err = conn.Exec(fmt.Sprintf(
284+
"PRAGMA user_version = %d", dataVersion,
285+
))
286+
require.NoError(t, err, "set current data version")
287+
require.NoError(t, conn.Close(), "close legacy database")
288+
289+
d, err := Open(path)
290+
require.ErrorContains(t, err, "injected parser parent backfill failure")
291+
require.Nil(t, d)
292+
293+
conn, err = sql.Open("sqlite3", makeDSN(path, false))
294+
require.NoError(t, err, "reopen failed migration")
295+
conn.SetMaxOpenConns(1)
296+
var columnCount int
297+
err = conn.QueryRow(`
298+
SELECT count(*) FROM pragma_table_info('sessions')
299+
WHERE name = 'parser_parent_session_id'
300+
`).Scan(&columnCount)
301+
require.NoError(t, err, "inspect schema after failed migration")
302+
assert.Zero(t, columnCount, "failed migration must roll back added column")
303+
_, err = conn.Exec(`DROP TRIGGER fail_parser_parent_backfill`)
304+
require.NoError(t, err, "remove injected migration failure")
305+
require.NoError(t, conn.Close(), "close failed migration database")
306+
307+
d, err = Open(path)
308+
require.NoError(t, err, "retry migration")
309+
defer d.Close()
310+
311+
var got sql.NullString
312+
err = d.getReader().QueryRow(`
313+
SELECT parser_parent_session_id FROM sessions WHERE id = 'kid'
314+
`).Scan(&got)
315+
require.NoError(t, err, "query retried parser parent")
316+
require.True(t, got.Valid, "retried parser parent must be set")
317+
assert.Equal(t, "parsed-parent", got.String, "retried parser parent")
318+
}
319+
264320
func TestLegacySchemaAddsArtifactImportAuthorityNonDestructively(t *testing.T) {
265321
path := filepath.Join(t.TempDir(), "legacy.db")
266322
conn, err := sql.Open("sqlite3", makeDSN(path, false))

0 commit comments

Comments
 (0)