Commit `0338e6ace3`

0338e6ace3de199d5fc383852649919b68ef3e42

parent: a4152dd0b2

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-28 08:44 UTC

store: run foreign_key_check inside the migration transaction, before commit

Ref #261

Layout: unified · split

CHANGELOG.org +4

   picked the same way the web page picks one (#268).
 - =repo show='s mirror table truncates =LAST SYNC= to the second, like
   every other timestamp in a view (#268).
+- A =-- foreign_keys: off= migration's =foreign_key_check= now runs
+  inside the migration's own transaction, before commit, so a
+  violation rolls the migration back instead of leaving the bad
+  schema and =user_version= already persisted (#261).
 * v1.36.0 — 2026-09-23

internal/store/store.go +80 −76

	@@ -189,51 +189,26 @@ func (s *Store) migrateTo(target int) error {
189	if err != nil {	189	if err != nil {
190	return err	190	return err
191	}	191	}
192	step := func(sqlText string, newVersion int, fkOff bool) (retErr error) {	192	for cur < target {
193	if !fkOff {	193	m := ms[cur]
194	tx, err := s.DB.Begin()	194	if err := s.migrateStep(m.up, m.version, m.upFKOff); err != nil {
195	if err != nil {	195	return fmt.Errorf("migration %d up: %w", m.version, err)
196	return err
197	}
198	defer tx.Rollback()
199	if _, err := tx.Exec(sqlText); err != nil {
200	return err
201	}
202	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
203	return err
204	}
205	return tx.Commit()
206	}
207
208	// A script whose first line is "-- foreign_keys: off" rebuilds a
209	// table that other tables reference (labels, milestones): with
210	// foreign keys on, the rebuild-by-rename loses the children's
211	// rows. PRAGMA foreign_keys is a no-op inside a transaction, and
212	// the pool gives no guarantee that a pragma set on one connection
213	// is seen by the connection Begin() draws next, so the whole step
214	// — pragma off, transaction, pragma on, foreign_key_check — runs
215	// on a single pinned connection.
216	ctx := context.Background()
217	conn, err := s.DB.Conn(ctx)
218	if err != nil {
219	return err
220	}
221	defer conn.Close()
222	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = OFF"); err != nil {
223	return err
224	}	196	}
225	// The connection goes back to the pool when this returns, so every	197	cur = m.version
226	// path out of here has to put foreign keys back on first.	198	}
227	restoreFK := func() error {	199	for cur > target {
228	_, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = ON")	200	m := ms[cur-1]
229	return err	201	if err := s.migrateStep(m.down, m.version-1, m.downFKOff); err != nil {
		202	return fmt.Errorf("migration %d down: %w", m.version, err)
230	}	203	}
231	defer func() {	204	cur = m.version - 1
232	if err := restoreFK(); err != nil && retErr == nil {	205	}
233	retErr = err	206	return nil
234	}	207	}
235	}()	208
236	tx, err := conn.BeginTx(ctx, nil)	209	func (s *Store) migrateStep(sqlText string, newVersion int, fkOff bool) (retErr error) {
		210	if !fkOff {
		211	tx, err := s.DB.Begin()
237	if err != nil {	212	if err != nil {
238	return err	213	return err
239	}	214	}
	@@ -244,44 +219,73 @@ func (s *Store) migrateTo(target int) error {
244	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {	219	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
245	return err	220	return err
246	}	221	}
247	if err := tx.Commit(); err != nil {	222	return tx.Commit()
248	return err
249	}
250	if err := restoreFK(); err != nil {
251	return err
252	}
253	rows, err := conn.QueryContext(ctx, "PRAGMA foreign_key_check")
254	if err != nil {
255	return err
256	}
257	defer rows.Close()
258	if rows.Next() {
259	var table string
260	var rowid sql.NullInt64
261	var referredTable string
262	var fkid int
263	if err := rows.Scan(&table, &rowid, &referredTable, &fkid); err != nil {
264	return err
265	}
266	return fmt.Errorf("foreign_key_check failed after migration: %s", table)
267	}
268	return rows.Err()
269	}	223	}
270	for cur < target {	224
271	m := ms[cur]	225	// A script whose first line is "-- foreign_keys: off" rebuilds a
272	if err := step(m.up, m.version, m.upFKOff); err != nil {	226	// table that other tables reference (labels, milestones): with
273	return fmt.Errorf("migration %d up: %w", m.version, err)	227	// foreign keys on, the rebuild-by-rename loses the children's
		228	// rows. PRAGMA foreign_keys is a no-op inside a transaction, and
		229	// the pool gives no guarantee that a pragma set on one connection
		230	// is seen by the connection Begin() draws next, so the whole step
		231	// — pragma off, transaction, foreign_key_check, commit, pragma on —
		232	// runs on a single pinned connection. The check runs before commit:
		233	// checking after would report a violation once the bad schema and
		234	// user_version were already persisted.
		235	ctx := context.Background()
		236	conn, err := s.DB.Conn(ctx)
		237	if err != nil {
		238	return err
		239	}
		240	defer conn.Close()
		241	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = OFF"); err != nil {
		242	return err
		243	}
		244	// The connection goes back to the pool when this returns, so every
		245	// path out of here has to put foreign keys back on first.
		246	defer func() {
		247	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil && retErr == nil {
		248	retErr = err
274	}	249	}
275	cur = m.version	250	}()
		251	tx, err := conn.BeginTx(ctx, nil)
		252	if err != nil {
		253	return err
276	}	254	}
277	for cur > target {	255	defer tx.Rollback()
278	m := ms[cur-1]	256	if _, err := tx.Exec(sqlText); err != nil {
279	if err := step(m.down, m.version-1, m.downFKOff); err != nil {	257	return err
280	return fmt.Errorf("migration %d down: %w", m.version, err)	258	}
		259	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
		260	return err
		261	}
		262	// foreign_key_check works with enforcement off: it inspects the data
		263	// directly rather than consulting the pragma. Running it here, inside
		264	// the transaction, means a violation rolls back the whole rebuild
		265	// (the deferred tx.Rollback fires) instead of leaving the bad schema
		266	// and version committed.
		267	rows, err := tx.QueryContext(ctx, "PRAGMA foreign_key_check")
		268	if err != nil {
		269	return err
		270	}
		271	if rows.Next() {
		272	var table string
		273	var rowid sql.NullInt64
		274	var referredTable string
		275	var fkid int
		276	if err := rows.Scan(&table, &rowid, &referredTable, &fkid); err != nil {
		277	rows.Close()
		278	return err
281	}	279	}
282	cur = m.version - 1	280	rows.Close()
		281	return fmt.Errorf("foreign_key_check failed after migration: %s row %v", table, rowid)
283	}	282	}
284	return nil	283	if err := rows.Err(); err != nil {
		284	rows.Close()
		285	return err
		286	}
		287	rows.Close()
		288	return tx.Commit()
285	}	289	}
286		290
287	// IsInternal reports whether err is the database or the I/O beneath it	291	// IsInternal reports whether err is the database or the I/O beneath it

internal/store/store_test.go +52

	@@ -255,3 +255,55 @@ func TestMigrationForeignKeysDirective(t *testing.T) {
255	t.Fatalf("foreign_keys after MigrateUp: %d, want 1", fk)	255	t.Fatalf("foreign_keys after MigrateUp: %d, want 1", fk)
256	}	256	}
257	}	257	}
		258
		259	// A migration marked "-- foreign_keys: off" must have its
		260	// foreign_key_check run before the transaction commits, not after —
		261	// otherwise a violation is reported once the bad schema and
		262	// user_version are already persisted (#261).
		263	func TestFKOffMigrationChecksBeforeCommit(t *testing.T) {
		264	s := open(t)
		265	if err := s.MigrateUp(); err != nil {
		266	t.Fatal(err)
		267	}
		268	versionBefore, err := s.Version()
		269	if err != nil {
		270	t.Fatal(err)
		271	}
		272
		273	// Insert a row a fkOff rebuild would have to preserve or complain
		274	// about: a milestone with no matching repo_id (the deliberately
		275	// impossible case a corrupt migration would produce).
		276	if _, err := s.DB.Exec("PRAGMA foreign_keys = OFF"); err != nil {
		277	t.Fatal(err)
		278	}
		279	if _, err := s.DB.Exec(
		280	"INSERT INTO milestones (repo_id, title, state, created_at) VALUES (99999, 'orphan', 'open', datetime('now'))"); err != nil {
		281	t.Fatal(err)
		282	}
		283	if _, err := s.DB.Exec("PRAGMA foreign_keys = ON"); err != nil {
		284	t.Fatal(err)
		285	}
		286
		287	// A no-op fkOff step (rewriting milestones to itself) must now
		288	// refuse — before it commits, not after — because the orphan row
		289	// fails foreign_key_check.
		290	err = s.migrateStep(
		291	"UPDATE sqlite_master SET name = name WHERE 0", versionBefore+1, true)
		292	if err == nil {
		293	t.Fatal("expected foreign_key_check to refuse the orphaned row")
		294	}
		295	after, err := s.Version()
		296	if err != nil {
		297	t.Fatal(err)
		298	}
		299	if after != versionBefore {
		300	t.Fatalf("user_version changed to %d despite the refused check (should stay %d)", after, versionBefore)
		301	}
		302	var fk int
		303	if err := s.DB.QueryRow("PRAGMA foreign_keys").Scan(&fk); err != nil {
		304	t.Fatal(err)
		305	}
		306	if fk != 1 {
		307	t.Fatalf("foreign_keys after refused fkOff step: %d, want 1", fk)
		308	}
		309	}

Commit 0338e6ace3

Commit `0338e6ace3`