From 6b4448b24508889078fce35635ebd30086797d6f Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Thu, 20 Aug 2026 14:56:48 +0200 Subject: [PATCH] =?UTF-8?q?fix(settings):=20AI,=20Gmail=20og=20kalenderen?= =?UTF-8?q?=20delte=20=C3=A9n=20r=C3=A6kke=20og=20slettede=20hinanden?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Meldt ind som "AI-informationerne gemmes ikke". De blev gemt. De blev taget bagefter. `user_settings` blev oprettet med `user_id` som hele sin primærnøgle — én række per person, uanset scope — og upserten skrev `scope` om ved konflikt. Der er tre scopes i live: `ai`, `gmail` og `calendar`. De delte den ene række, så hver skrivning slettede de to andre, og hver eneste svarede 204. Det virker begge veje: at forbinde kalenderen slettede Gmail-forbindelsen, og Gmail slettede kalenderen. Fælden var set komme to gange. 0011 gav instansens indstillinger deres egen tabel delvis af den grund og skrev advarslen ned med rene ord; 0014 flyttede postkasserne ud og skrev, at advarslen "nær var trådt i for anden gang". Tredje gang var kalenderen, som lagde et `calendar`-scope ved siden af de to andre. Lektien værd at holde fast i: en advarsel i en migration bliver læst af den, der redigerer *den* migration, og det var ingen. Formen er rettet her, så der ikke er noget tilbage at advare om — `PRIMARY KEY (user_id, scope)`, og upserten rammer den nøgle. Tabellen bygges om, fordi SQLite ikke kan ændre en primærnøgle på plads. De rækker, der findes, kopieres som de er: hver person har højst én, og den holder det scope, der vandt sidst. Det, de tidligere skrivninger tog, er overskrevet og kan ikke hentes tilbage her — efter opdateringen skal AI-udbyderen sættes op igen, og Gmail-forbindelsen tjekkes. To prøver: én mod store-laget, hvor formen var fejlen, og én gennem handlerne, hvor den blev set. Co-Authored-By: Claude Opus 5 --- internal/httpapi/integrations_test.go | 42 +++++++++++++ internal/store/mailboxes_test.go | 6 +- .../0025_user_settings_per_scope.sql | 37 +++++++++++ internal/store/notifications.go | 8 ++- internal/store/secrets_test.go | 62 +++++++++++++++++++ 5 files changed, 152 insertions(+), 3 deletions(-) create mode 100644 internal/store/migrations/0025_user_settings_per_scope.sql diff --git a/internal/httpapi/integrations_test.go b/internal/httpapi/integrations_test.go index 9009bf7..8aea0b5 100644 --- a/internal/httpapi/integrations_test.go +++ b/internal/httpapi/integrations_test.go @@ -562,6 +562,48 @@ func TestAIKeyIsNeverReturned(t *testing.T) { } } +// Connecting something else must not delete the AI provider. +// +// Reported from use as "the AI settings are not saved": they were saved, and then +// taken. `user_settings` had `user_id` as its whole primary key, and the upsert +// renamed `scope` on conflict — so the `ai`, `gmail` and `calendar` scopes shared +// one row and each write deleted the other two. Every write returned 204. +// +// Through the handlers, because that is where it was seen. The shape is asserted +// against the store as well: see TestOnePersonCanHaveSettingsForMoreThanOneThing. +func TestTheAIProviderSurvivesConnectingSomethingElse(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + ts.do(t, "PUT", "/api/v1/ai/settings", map[string]any{ + "provider": "anthropic", "model": "claude-sonnet-5", "api_key": "sk-hemmelig-nøgle", + }) + + user, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk") + if err != nil { + t.Fatal(err) + } + // What the calendar and the mailbox handlers do when a handshake starts: write + // their own scope for the same person. + for _, scope := range []string{"calendar", "gmail"} { + if err := ts.db.SetUserSettings(t.Context(), user.ID, scope, + map[string]any{"pkce_state": "s-" + scope}); err != nil { + t.Fatalf("save %s: %v", scope, err) + } + } + + _, body := ts.do(t, "GET", "/api/v1/ai/settings", nil) + if body["provider"] != "anthropic" { + t.Errorf("the provider is %v after connecting two other things, want anthropic", body["provider"]) + } + if body["model"] != "claude-sonnet-5" { + t.Errorf("the model is %v", body["model"]) + } + if body["has_key"] != true { + t.Error("the key was taken with it") + } +} + func TestGmailSettings(t *testing.T) { ts := newTestServer(t) ts.bootstrap(t) diff --git a/internal/store/mailboxes_test.go b/internal/store/mailboxes_test.go index a258ed0..2913edd 100644 --- a/internal/store/mailboxes_test.go +++ b/internal/store/mailboxes_test.go @@ -195,10 +195,14 @@ func TestAGmailConnectedBeforeTheMoveSurvivesIt(t *testing.T) { } values := `{"refresh_token":"` + sealed + `","trigger":"label","label":"Verdande",` + `"email":"kw@nolimit.dk","seen":["abc","def"],"project_id":"p-1"}` + // The conflict target is (user_id, scope) since 0025 — before that the table + // held one row per person and the upsert renamed its scope, which is the bug + // 0025 exists to undo. Nothing conflicts here; the clause only has to be legal + // against the schema this test is running on. if _, err := db.ExecContext(context.Background(), ` INSERT INTO user_settings (user_id, scope, values_json, updated_at) VALUES (?, 'gmail', ?, 0) - ON CONFLICT (user_id) DO UPDATE SET scope = 'gmail', values_json = excluded.values_json`, + ON CONFLICT (user_id, scope) DO UPDATE SET values_json = excluded.values_json`, userID, values); err != nil { t.Fatal(err) } diff --git a/internal/store/migrations/0025_user_settings_per_scope.sql b/internal/store/migrations/0025_user_settings_per_scope.sql new file mode 100644 index 0000000..c9ab3b0 --- /dev/null +++ b/internal/store/migrations/0025_user_settings_per_scope.sql @@ -0,0 +1,37 @@ +-- One row per person *and scope*, which is what the table always meant. +-- +-- `user_settings` was created with `user_id` as its whole primary key, and its +-- upsert rewrote `scope` on conflict. So a person had exactly one settings row, +-- and whichever scope was written last was the only one that survived: saving an +-- AI provider deleted the Gmail connection, and connecting a calendar deleted the +-- AI provider. Nothing reported it — the write succeeded, and the loss was in the +-- row it landed on. +-- +-- This was seen coming twice. Migration 0011 gave the instance's settings their +-- own table partly for this reason and wrote the warning down in as many words; +-- 0014 moved mailboxes out and said the warning "was nearly walked into a second +-- time". The third time was the calendar, which added a `calendar` scope beside +-- `ai` and `gmail` — and walked in. +-- +-- The lesson worth keeping: a warning in a migration is read by whoever is editing +-- *that* migration, and nobody was. The shape is fixed here so there is nothing +-- left to warn about. +-- +-- SQLite cannot change a primary key in place, so the table is rebuilt. Existing +-- rows are copied as they are: each person has at most one, and it holds whatever +-- scope won last. What the earlier writes lost is already gone and cannot be +-- recovered here — the values were overwritten, not orphaned. +CREATE TABLE user_settings_new ( + user_id TEXT NOT NULL REFERENCES users (id) ON DELETE CASCADE, + scope TEXT NOT NULL DEFAULT 'general', + values_json TEXT NOT NULL DEFAULT '{}', + updated_at INTEGER NOT NULL, + PRIMARY KEY (user_id, scope) +); + +INSERT INTO user_settings_new (user_id, scope, values_json, updated_at) +SELECT user_id, scope, values_json, updated_at FROM user_settings; + +DROP TABLE user_settings; + +ALTER TABLE user_settings_new RENAME TO user_settings; diff --git a/internal/store/notifications.go b/internal/store/notifications.go index f422520..9df17de 100644 --- a/internal/store/notifications.go +++ b/internal/store/notifications.go @@ -391,11 +391,15 @@ func (db *DB) SetUserSettings(ctx context.Context, userID, scope string, values if err != nil { return err } + // On (user_id, scope), which is the key since 0025. It used to be on `user_id` + // alone and set `scope = excluded.scope` — so writing one scope did not add a + // row beside the others, it took theirs and renamed it. The `ai`, `gmail` and + // `calendar` settings each deleted the other two, and the write reported + // success every time. _, err = db.ExecContext(ctx, ` INSERT INTO user_settings (user_id, scope, values_json, updated_at) VALUES (?, ?, ?, ?) - ON CONFLICT (user_id) DO UPDATE SET - scope = excluded.scope, + ON CONFLICT (user_id, scope) DO UPDATE SET values_json = excluded.values_json, updated_at = excluded.updated_at`, userID, scope, string(raw), time.Now().Unix()) diff --git a/internal/store/secrets_test.go b/internal/store/secrets_test.go index 5f8435c..bda231f 100644 --- a/internal/store/secrets_test.go +++ b/internal/store/secrets_test.go @@ -124,3 +124,65 @@ func sealedStore(t *testing.T) (*DB, string) { } return db, user.ID } + +// Three integrations, one person, and none of them may delete the others. +// +// `user_settings` had `user_id` as its whole primary key and an upsert that +// rewrote `scope` on conflict, so every person had exactly one settings row and +// the last write won the lot: saving an AI provider deleted the Gmail connection, +// and connecting a calendar deleted the AI provider. The write reported success +// each time, which is why it was reported as "the AI settings are not saved" +// rather than as "connecting the calendar deleted them". +// +// Written against the store rather than the handlers, because the shape was the +// bug: every caller was correct on its own. +func TestOnePersonCanHaveSettingsForMoreThanOneThing(t *testing.T) { + db, userID := sealedStore(t) + ctx := context.Background() + + want := map[string]map[string]any{ + "ai": {"provider": "anthropic", "model": "claude-sonnet-5", "api_key": "sk-ant-xxx"}, + "gmail": {"refresh_token": "1//0-gmail", "label": "Verdande"}, + "calendar": {"refresh_token": "1//0-calendar", "calendar_id": "primary"}, + } + // In this order on purpose: the AI settings are written first and read last, + // so a scope that steals the row is caught by the one written earliest. + for _, scope := range []string{"ai", "gmail", "calendar"} { + if err := db.SetUserSettings(ctx, userID, scope, want[scope]); err != nil { + t.Fatalf("save %s: %v", scope, err) + } + } + + for _, scope := range []string{"ai", "gmail", "calendar"} { + got, err := db.UserSettings(ctx, userID, scope) + if err != nil { + t.Fatalf("read %s: %v", scope, err) + } + for key, value := range want[scope] { + if got[key] != value { + t.Errorf("%s.%s is %v, want %v", scope, key, got[key], value) + } + } + } + + // And a second write to one scope updates it rather than adding another row — + // the upsert still has to upsert. + if err := db.SetUserSettings(ctx, userID, "ai", map[string]any{"provider": "openai"}); err != nil { + t.Fatal(err) + } + var rows int + if err := db.QueryRowContext(ctx, + `SELECT count(*) FROM user_settings WHERE user_id = ?`, userID).Scan(&rows); err != nil { + t.Fatal(err) + } + if rows != 3 { + t.Errorf("the person has %d settings rows, want 3", rows) + } + back, err := db.UserSettings(ctx, userID, "ai") + if err != nil { + t.Fatal(err) + } + if back["provider"] != "openai" { + t.Errorf("the second write did not land: %v", back["provider"]) + } +}