kriss/geniusrun
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

store: fix migration 0026's silent-cascade-delete FK toggle bug

Browse Source 
Migration 0026 (activities table rebuild for the per-user unique
constraint) issued its own PRAGMA foreign_keys = OFF/ON inside the
migration's SQL content, but the whole migration file runs inside one
db.go tx.Begin()/tx.Exec()/tx.Commit() transaction, and SQLite documents
PRAGMA foreign_keys as a no-op once a transaction is open. As a result FK
enforcement never actually got disabled, so DROP TABLE activities
triggered SQLite's implicit DELETE FROM semantics, firing ON DELETE
CASCADE on every row in laps, activity_samples, and kind_assignments for
every activity -- silently, with no error. On a real upgrade with synced
data this would have permanently destroyed all lap/sample/classification
history. It was masked because every existing migration test runs
migrations back-to-back on an empty temp database with no pre-existing
child rows.

Fix: add "0026_activities_unique_constraint.sql" to db.go's
tableRebuildMigrations map so it gets the same autocommit-mode FK
disable/enable toggle (before/after the transaction) already used for
migrations 0023/0024/0025, and remove the now-redundant/misleading
mid-transaction PRAGMA lines from the migration file itself, matching
the established pattern.

Also restore the DEFAULT '' on event_type_key in migration 0026's
rebuilt activities table -- it was dropped from migration 0007's
original column definition during the rebuild, which broke every insert
that omits event_type_key and relies on that default
(TestClaimLegacyOwner_BindsExistingSingletonRowsToOneNewUser and others).

Extend TestRebuildMigrationsPreserveForeignKeyReferences to cover 0026:
seed a laps row and an activity_samples row (in addition to the existing
kind_assignments row) against a pre-existing activities row before the
rebuild migrations run, then verify after 0023-0026 complete that all
three child rows still exist and still reference the same activity, and
that FK enforcement rejects bogus activity_id/workout_kind_id afterward.
This is the regression guard that would have caught the original bug.

Note: TestClaimLegacyOwner_NoOpOnGenuinelyFreshInstall still fails on
this branch; verified it fails identically at the prior commit
(d8b7228), so it's a pre-existing, unrelated bug in ClaimLegacyOwner
(not migration 0026 or this FK-toggle bug) and out of scope for this fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Kriss Vila
2026-07-25 14:28:16 +02:00
parent d8b722820d
commit b08f0152ef
3 changed files with 109 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
backend/internal/store/db.go
View File

@@ -79,16 +79,19 @@ func (db *DB) migrate() error {
return fmt.Errorf("read migration %s: %w", name, err)
}
// Migrations 0023, 0024, and 0025 rebuild tables that have incoming
// foreign keys (kind_assignments/workout_type_paces reference
// workout_kinds; sync_state is referenced by activities/sync_runs).
// These require temporary FK disable in autocommit mode (before the
// transaction begins) so the DROP TABLE succeeds; a mid-transaction
// PRAGMA is a no-op with modernc.org/sqlite.
// Migrations 0023, 0024, 0025, and 0026 rebuild tables that have
// incoming foreign keys (kind_assignments/workout_type_paces
// reference workout_kinds; sync_state is referenced by
// activities/sync_runs; laps/activity_samples/kind_assignments
// reference activities). These require temporary FK disable in
// autocommit mode (before the transaction begins) so the DROP
// TABLE succeeds; a mid-transaction PRAGMA is a no-op with
// modernc.org/sqlite.
tableRebuildMigrations := map[string]bool{
"0023_profile_user_scoped.sql": true,
"0024_workout_kinds_user_scoped.sql": true,
"0025_sync_state_user_scoped.sql": true,
"0023_profile_user_scoped.sql": true,
"0024_workout_kinds_user_scoped.sql": true,
"0025_sync_state_user_scoped.sql": true,
"0026_activities_unique_constraint.sql": true,
}
needsFKToggle := tableRebuildMigrations[name]

17
backend/internal/store/migrations/0026_activities_unique_constraint.sql
View File

@@ -4,13 +4,24 @@
-- constraint alone was intentionally left in place by migration 0022 as a
-- belts-and-suspenders safeguard (Garmin IDs are globally unique in practice),
-- but it must be removed now to allow Task 6's cross-user test to pass.
PRAGMA foreign_keys = OFF;
--
-- The FK enforcement toggle needed for this rebuild (DROP TABLE on a table
-- with real inbound foreign keys) happens in Go code around this migration's
-- execution (db.go's tableRebuildMigrations map), in autocommit mode before
-- the transaction begins -- a mid-transaction PRAGMA foreign_keys statement
-- is a documented no-op with modernc.org/sqlite, so it must not appear here.
--
-- event_type_key keeps the DEFAULT '' that migration 0007 originally gave
-- it (ALTER TABLE ... ADD COLUMN event_type_key TEXT NOT NULL DEFAULT '');
-- dropping the default here (as an earlier draft of this migration did)
-- broke every INSERT that omits event_type_key and relies on that default,
-- which several existing tests (e.g. TestClaimLegacyOwner_*) do.
CREATE TABLE activities_new (
id INTEGER PRIMARY KEY AUTOINCREMENT,
user_id INTEGER REFERENCES users(id),
garmin_activity_id INTEGER NOT NULL,
event_type_key TEXT NOT NULL,
event_type_key TEXT NOT NULL DEFAULT '',
workout_id INTEGER,
start_time_utc TEXT NOT NULL,
duration_seconds REAL NOT NULL,
@@ -42,5 +53,3 @@ ALTER TABLE activities_new RENAME TO activities;
CREATE INDEX idx_activities_start_time ON activities(start_time_utc);
CREATE INDEX idx_activities_user_garmin_id ON activities(user_id, garmin_activity_id);
PRAGMA foreign_keys = ON;

102
backend/internal/store/store_test.go
View File

@@ -343,19 +343,27 @@ func TestForeignKeyEnforcementPostMigration(t *testing.T) {
// TestRebuildMigrationsPreserveForeignKeyReferences proves the specific
// property none of the other migration tests cover: every other test opens
// a fresh DB via store.Open, which runs migrations 0001-0025 in one
// uninterrupted pass over an empty database, so migrations 0023/0024/0025's
// a fresh DB via store.Open, which runs migrations 0001-0026 in one
// uninterrupted pass over an empty database, so migrations 0023/0024/0025/0026's
// table-rebuild (create-new/copy/drop-old/rename-into-place) never has any
// real pre-existing rows to carry across. A real single-tenant deployment
// upgrading to this schema version has months of synced activities and
// kind_assignments rows referencing real workout_kinds rows by foreign key
// (per this repo's CLAUDE.md: one profile, real Garmin history, a review
// queue with manual overrides already recorded). This test manually applies
// migrations up through 0022, inserts rows simulating that pre-existing
// install, then applies 0023/0024/0025 and verifies the FK-referencing row
// still resolves correctly -- and that FK enforcement is genuinely back on
// afterward -- rather than just checking that migrations apply to an empty
// DB without erroring.
// upgrading to this schema version has months of synced activities, laps,
// activity_samples, and kind_assignments rows referencing real activities/
// workout_kinds rows by foreign key (per this repo's CLAUDE.md: one profile,
// real Garmin history, a review queue with manual overrides already
// recorded). This test manually applies migrations up through 0022, inserts
// rows simulating that pre-existing install, then applies 0023/0024/0025/0026
// and verifies every FK-referencing row still resolves correctly -- and that
// FK enforcement is genuinely back on afterward -- rather than just checking
// that migrations apply to an empty DB without erroring. This is the
// regression guard for a real bug: migration 0026 (activities table rebuild)
// originally issued its own mid-transaction `PRAGMA foreign_keys = OFF/ON`,
// which SQLite documents as a no-op once a transaction is open, so
// `DROP TABLE activities` silently cascade-deleted every laps/
// activity_samples/kind_assignments row for every activity via
// `ON DELETE CASCADE` -- with no error at all. It was masked because every
// other test runs migrations back-to-back on an empty database with no
// pre-existing child rows.
func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) {
ctx := context.Background()
path := filepath.Join(t.TempDir(), "geniusrun_rebuild_test.db")
@@ -377,13 +385,14 @@ func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) {
}
rebuildMigrations := map[string]bool{
"0023_profile_user_scoped.sql": true,
"0024_workout_kinds_user_scoped.sql": true,
"0025_sync_state_user_scoped.sql": true,
"0023_profile_user_scoped.sql": true,
"0024_workout_kinds_user_scoped.sql": true,
"0025_sync_state_user_scoped.sql": true,
"0026_activities_unique_constraint.sql": true,
}
// Mirrors db.go's migrate() method: FK toggle in autocommit mode around
// the transaction, only for the three table-rebuild migrations.
// the transaction, only for the four table-rebuild migrations.
applyMigration := func(name string) {
t.Helper()
content, err := migrationsFS.ReadFile("migrations/" + name)
@@ -470,12 +479,35 @@ func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) {
t.Fatalf("kind_assignments LastInsertId: %v", err)
}
// Now apply the three rebuild migrations that drop/recreate profile,
// workout_kinds, and sync_state.
res, err = sqlDB.ExecContext(ctx, `
INSERT INTO laps (activity_id, lap_index, raw_json)
VALUES (?, 0, '{}')`, activityID)
if err != nil {
t.Fatalf("insert pre-existing laps row: %v", err)
}
lapID, err := res.LastInsertId()
if err != nil {
t.Fatalf("laps LastInsertId: %v", err)
}
res, err = sqlDB.ExecContext(ctx, `
INSERT INTO activity_samples (activity_id, elapsed_seconds, timestamp_ms, heart_rate)
VALUES (?, 60, 1735711260000, 150)`, activityID)
if err != nil {
t.Fatalf("insert pre-existing activity_samples row: %v", err)
}
sampleID, err := res.LastInsertId()
if err != nil {
t.Fatalf("activity_samples LastInsertId: %v", err)
}
// Now apply the four rebuild migrations that drop/recreate profile,
// workout_kinds, sync_state, and (0026) activities itself.
for _, name := range []string{
"0023_profile_user_scoped.sql",
"0024_workout_kinds_user_scoped.sql",
"0025_sync_state_user_scoped.sql",
"0026_activities_unique_constraint.sql",
} {
applyMigration(name)
}
@@ -493,13 +525,47 @@ func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) {
t.Fatalf("expected pre-existing kind_assignment to still resolve to 'Pre-Existing Custom Kind' after rebuild, got %q", resolvedName)
}
// The pre-existing laps row must still exist and still reference the
// same activity -- this is the exact regression guard for migration
// 0026's DROP TABLE activities silently cascade-deleting laps via
// ON DELETE CASCADE when FK enforcement wasn't actually disabled.
var lapActivityID int64
if err := sqlDB.QueryRowContext(ctx, `
SELECT activity_id FROM laps WHERE id = ?`, lapID).Scan(&lapActivityID); err != nil {
t.Fatalf("query pre-existing laps row after rebuild: %v", err)
}
if lapActivityID != activityID {
t.Fatalf("expected pre-existing laps row to still reference activity %d after rebuild, got %d", activityID, lapActivityID)
}
// Same guard for activity_samples.
var sampleActivityID int64
if err := sqlDB.QueryRowContext(ctx, `
SELECT activity_id FROM activity_samples WHERE id = ?`, sampleID).Scan(&sampleActivityID); err != nil {
t.Fatalf("query pre-existing activity_samples row after rebuild: %v", err)
}
if sampleActivityID != activityID {
t.Fatalf("expected pre-existing activity_samples row to still reference activity %d after rebuild, got %d", activityID, sampleActivityID)
}
// FK enforcement must be genuinely active again post-migration: a bogus
// workout_kind_id must be rejected, not silently accepted.
// workout_kind_id, activity_id (laps), and activity_id (activity_samples)
// must all be rejected, not silently accepted.
if _, err := sqlDB.ExecContext(ctx, `
INSERT INTO kind_assignments (activity_id, workout_kind_id, assignment_source, status)
VALUES (?, 999999, 'rule_engine', 'assigned')`, activityID); err == nil {
t.Fatal("expected FK constraint violation for nonexistent workout_kind_id after rebuild, but insert succeeded")
}
if _, err := sqlDB.ExecContext(ctx, `
INSERT INTO laps (activity_id, lap_index, raw_json)
VALUES (999999, 0, '{}')`); err == nil {
t.Fatal("expected FK constraint violation for nonexistent activity_id in laps after rebuild, but insert succeeded")
}
if _, err := sqlDB.ExecContext(ctx, `
INSERT INTO activity_samples (activity_id, elapsed_seconds, timestamp_ms, heart_rate)
VALUES (999999, 60, 1735711260000, 150)`); err == nil {
t.Fatal("expected FK constraint violation for nonexistent activity_id in activity_samples after rebuild, but insert succeeded")
}
}
func contains(s string, substrs ...string) bool {