From ad1906589d5a794de86b94ecf22af19448a471d1 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Wed, 22 Apr 2026 19:34:34 +1000 Subject: [PATCH] fix(coderd): allow deleting chat providers used in historical chats (#24568) Drop the `chat_model_configs.provider -> chat_providers.provider` foreign key and soft-delete model configs when their provider is removed. The provider row is now hard-deleted inside a transaction that also tombstones its model configs and promotes a replacement default when needed. Historical chats and messages keep pointing at the soft-deleted model config rows, which are hidden from live/admin queries but still resolve for read. The runtime chat path already falls back to the default model config when a soft-deleted config is looked up. Replaces the lost FK validation in the create/update model-config handlers with an explicit provider lookup that returns the existing `Chat provider is not configured.` 400. ## UX **Admin deleting a chat provider that has historical usage** - Before: blocked with 400 `Provider models are still referenced by existing chats.` Admins had no in-product way to remove a provider that had ever been used. - After: delete succeeds (204). Any model configs under that provider are soft-deleted. If the removed provider owned the default model config, one of the remaining live configs is auto-promoted to the new default. The promotion is deterministic (`ensureDefaultChatModelConfig` picks the first live config by `provider ASC, model ASC, updated_at DESC, id DESC`); there is no picker, and no toast or response detail names which config became the new default. **End users with chats that used a deleted provider's model** - Old chats still open and their history still renders unchanged. - Sending a new turn in such a chat silently falls back to the current default model. No banner or warning tells the user the original model is gone. - The model picker no longer lists the deleted model. - If no default model config exists at all after the delete, sending a new turn fails with `no default chat model config is available`. **Admin creating or updating a model config against a provider that is not configured** - Same as before: 400 `Chat provider is not configured.` Only the detection mechanism changed (explicit `FOR UPDATE` lookup inside the transaction, which also serializes against a concurrent provider delete). **Admin updating a model config whose row disappears mid-transaction** - Now returns the standard 404 `Resource not found or you do not have access to this resource` instead of the previous 500 that leaked `sql: no rows in result set` in the detail. Unrelated internal races (for example a race on the promoted default candidate) are still reported as 500 so they are not misclassified as "your target is gone". Closes CODAGT-23 --- coderd/database/dbauthz/dbauthz.go | 21 ++ coderd/database/dbauthz/dbauthz_test.go | 16 + coderd/database/dbmetrics/querymetrics.go | 24 ++ coderd/database/dbmock/dbmock.go | 44 +++ coderd/database/dump.sql | 3 - coderd/database/foreign_key_constraint.go | 1 - ...rop_chat_model_config_provider_fk.down.sql | 27 ++ ..._drop_chat_model_config_provider_fk.up.sql | 2 + coderd/database/querier.go | 3 + coderd/database/queries.sql.go | 79 +++++ coderd/database/queries/chatmodelconfigs.sql | 11 + coderd/database/queries/chatproviders.sql | 18 + coderd/exp_chats.go | 93 ++++-- coderd/exp_chats_test.go | 312 ++++++++++++++++++ 14 files changed, 628 insertions(+), 26 deletions(-) create mode 100644 coderd/database/migrations/000474_drop_chat_model_config_provider_fk.down.sql create mode 100644 coderd/database/migrations/000474_drop_chat_model_config_provider_fk.up.sql diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index f9bb74edc9..d2b103624f 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1889,6 +1889,13 @@ func (q *querier) DeleteChatModelConfigByID(ctx context.Context, id uuid.UUID) e return q.db.DeleteChatModelConfigByID(ctx, id) } +func (q *querier) DeleteChatModelConfigsByProvider(ctx context.Context, provider string) error { + if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceDeploymentConfig); err != nil { + return err + } + return q.db.DeleteChatModelConfigsByProvider(ctx, provider) +} + func (q *querier) DeleteChatProviderByID(ctx context.Context, id uuid.UUID) error { if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceDeploymentConfig); err != nil { return err @@ -2827,6 +2834,13 @@ func (q *querier) GetChatProviderByID(ctx context.Context, id uuid.UUID) (databa return q.db.GetChatProviderByID(ctx, id) } +func (q *querier) GetChatProviderByIDForUpdate(ctx context.Context, id uuid.UUID) (database.ChatProvider, error) { + if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceDeploymentConfig); err != nil { + return database.ChatProvider{}, err + } + return q.db.GetChatProviderByIDForUpdate(ctx, id) +} + func (q *querier) GetChatProviderByProvider(ctx context.Context, provider string) (database.ChatProvider, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceDeploymentConfig); err != nil { return database.ChatProvider{}, err @@ -2834,6 +2848,13 @@ func (q *querier) GetChatProviderByProvider(ctx context.Context, provider string return q.db.GetChatProviderByProvider(ctx, provider) } +func (q *querier) GetChatProviderByProviderForUpdate(ctx context.Context, provider string) (database.ChatProvider, error) { + if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceDeploymentConfig); err != nil { + return database.ChatProvider{}, err + } + return q.db.GetChatProviderByProviderForUpdate(ctx, provider) +} + func (q *querier) GetChatProviders(ctx context.Context) ([]database.ChatProvider, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceDeploymentConfig); err != nil { return nil, err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 326bbec5f2..7a7ee8f219 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -449,6 +449,11 @@ func (s *MethodTestSuite) TestChats() { dbm.EXPECT().DeleteChatModelConfigByID(gomock.Any(), id).Return(nil).AnyTimes() check.Args(id).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate) })) + s.Run("DeleteChatModelConfigsByProvider", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { + providerName := "test-provider" + dbm.EXPECT().DeleteChatModelConfigsByProvider(gomock.Any(), providerName).Return(nil).AnyTimes() + check.Args(providerName).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate) + })) s.Run("DeleteChatProviderByID", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { id := uuid.New() dbm.EXPECT().DeleteChatProviderByID(gomock.Any(), id).Return(nil).AnyTimes() @@ -803,12 +808,23 @@ func (s *MethodTestSuite) TestChats() { dbm.EXPECT().GetChatProviderByID(gomock.Any(), provider.ID).Return(provider, nil).AnyTimes() check.Args(provider.ID).Asserts(rbac.ResourceDeploymentConfig, policy.ActionRead).Returns(provider) })) + s.Run("GetChatProviderByIDForUpdate", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + provider := testutil.Fake(s.T(), faker, database.ChatProvider{}) + dbm.EXPECT().GetChatProviderByIDForUpdate(gomock.Any(), provider.ID).Return(provider, nil).AnyTimes() + check.Args(provider.ID).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate).Returns(provider) + })) s.Run("GetChatProviderByProvider", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { providerName := "test-provider" provider := testutil.Fake(s.T(), faker, database.ChatProvider{Provider: providerName}) dbm.EXPECT().GetChatProviderByProvider(gomock.Any(), providerName).Return(provider, nil).AnyTimes() check.Args(providerName).Asserts(rbac.ResourceDeploymentConfig, policy.ActionRead).Returns(provider) })) + s.Run("GetChatProviderByProviderForUpdate", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + providerName := "test-provider" + provider := testutil.Fake(s.T(), faker, database.ChatProvider{Provider: providerName}) + dbm.EXPECT().GetChatProviderByProviderForUpdate(gomock.Any(), providerName).Return(provider, nil).AnyTimes() + check.Args(providerName).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate).Returns(provider) + })) s.Run("GetChatProviders", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { providerA := testutil.Fake(s.T(), faker, database.ChatProvider{}) providerB := testutil.Fake(s.T(), faker, database.ChatProvider{}) diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 553871fb2d..cda2c66bbc 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -440,6 +440,14 @@ func (m queryMetricsStore) DeleteChatModelConfigByID(ctx context.Context, id uui return r0 } +func (m queryMetricsStore) DeleteChatModelConfigsByProvider(ctx context.Context, provider string) error { + start := time.Now() + r0 := m.s.DeleteChatModelConfigsByProvider(ctx, provider) + m.queryLatencies.WithLabelValues("DeleteChatModelConfigsByProvider").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteChatModelConfigsByProvider").Inc() + return r0 +} + func (m queryMetricsStore) DeleteChatProviderByID(ctx context.Context, id uuid.UUID) error { start := time.Now() r0 := m.s.DeleteChatProviderByID(ctx, id) @@ -1344,6 +1352,14 @@ func (m queryMetricsStore) GetChatProviderByID(ctx context.Context, id uuid.UUID return r0, r1 } +func (m queryMetricsStore) GetChatProviderByIDForUpdate(ctx context.Context, id uuid.UUID) (database.ChatProvider, error) { + start := time.Now() + r0, r1 := m.s.GetChatProviderByIDForUpdate(ctx, id) + m.queryLatencies.WithLabelValues("GetChatProviderByIDForUpdate").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetChatProviderByIDForUpdate").Inc() + return r0, r1 +} + func (m queryMetricsStore) GetChatProviderByProvider(ctx context.Context, provider string) (database.ChatProvider, error) { start := time.Now() r0, r1 := m.s.GetChatProviderByProvider(ctx, provider) @@ -1352,6 +1368,14 @@ func (m queryMetricsStore) GetChatProviderByProvider(ctx context.Context, provid return r0, r1 } +func (m queryMetricsStore) GetChatProviderByProviderForUpdate(ctx context.Context, provider string) (database.ChatProvider, error) { + start := time.Now() + r0, r1 := m.s.GetChatProviderByProviderForUpdate(ctx, provider) + m.queryLatencies.WithLabelValues("GetChatProviderByProviderForUpdate").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetChatProviderByProviderForUpdate").Inc() + return r0, r1 +} + func (m queryMetricsStore) GetChatProviders(ctx context.Context) ([]database.ChatProvider, error) { start := time.Now() r0, r1 := m.s.GetChatProviders(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index e518e772cc..e7bedcf107 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -715,6 +715,20 @@ func (mr *MockStoreMockRecorder) DeleteChatModelConfigByID(ctx, id any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteChatModelConfigByID", reflect.TypeOf((*MockStore)(nil).DeleteChatModelConfigByID), ctx, id) } +// DeleteChatModelConfigsByProvider mocks base method. +func (m *MockStore) DeleteChatModelConfigsByProvider(ctx context.Context, provider string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteChatModelConfigsByProvider", ctx, provider) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteChatModelConfigsByProvider indicates an expected call of DeleteChatModelConfigsByProvider. +func (mr *MockStoreMockRecorder) DeleteChatModelConfigsByProvider(ctx, provider any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteChatModelConfigsByProvider", reflect.TypeOf((*MockStore)(nil).DeleteChatModelConfigsByProvider), ctx, provider) +} + // DeleteChatProviderByID mocks base method. func (m *MockStore) DeleteChatProviderByID(ctx context.Context, id uuid.UUID) error { m.ctrl.T.Helper() @@ -2477,6 +2491,21 @@ func (mr *MockStoreMockRecorder) GetChatProviderByID(ctx, id any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatProviderByID", reflect.TypeOf((*MockStore)(nil).GetChatProviderByID), ctx, id) } +// GetChatProviderByIDForUpdate mocks base method. +func (m *MockStore) GetChatProviderByIDForUpdate(ctx context.Context, id uuid.UUID) (database.ChatProvider, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetChatProviderByIDForUpdate", ctx, id) + ret0, _ := ret[0].(database.ChatProvider) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetChatProviderByIDForUpdate indicates an expected call of GetChatProviderByIDForUpdate. +func (mr *MockStoreMockRecorder) GetChatProviderByIDForUpdate(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatProviderByIDForUpdate", reflect.TypeOf((*MockStore)(nil).GetChatProviderByIDForUpdate), ctx, id) +} + // GetChatProviderByProvider mocks base method. func (m *MockStore) GetChatProviderByProvider(ctx context.Context, provider string) (database.ChatProvider, error) { m.ctrl.T.Helper() @@ -2492,6 +2521,21 @@ func (mr *MockStoreMockRecorder) GetChatProviderByProvider(ctx, provider any) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatProviderByProvider", reflect.TypeOf((*MockStore)(nil).GetChatProviderByProvider), ctx, provider) } +// GetChatProviderByProviderForUpdate mocks base method. +func (m *MockStore) GetChatProviderByProviderForUpdate(ctx context.Context, provider string) (database.ChatProvider, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetChatProviderByProviderForUpdate", ctx, provider) + ret0, _ := ret[0].(database.ChatProvider) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetChatProviderByProviderForUpdate indicates an expected call of GetChatProviderByProviderForUpdate. +func (mr *MockStoreMockRecorder) GetChatProviderByProviderForUpdate(ctx, provider any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatProviderByProviderForUpdate", reflect.TypeOf((*MockStore)(nil).GetChatProviderByProviderForUpdate), ctx, provider) +} + // GetChatProviders mocks base method. func (m *MockStore) GetChatProviders(ctx context.Context) ([]database.ChatProvider, error) { m.ctrl.T.Helper() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index c41bcb9c8b..8a293ecca1 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -4161,9 +4161,6 @@ ALTER TABLE ONLY chat_messages ALTER TABLE ONLY chat_model_configs ADD CONSTRAINT chat_model_configs_created_by_fkey FOREIGN KEY (created_by) REFERENCES users(id); -ALTER TABLE ONLY chat_model_configs - ADD CONSTRAINT chat_model_configs_provider_fkey FOREIGN KEY (provider) REFERENCES chat_providers(provider) ON DELETE CASCADE; - ALTER TABLE ONLY chat_model_configs ADD CONSTRAINT chat_model_configs_updated_by_fkey FOREIGN KEY (updated_by) REFERENCES users(id); diff --git a/coderd/database/foreign_key_constraint.go b/coderd/database/foreign_key_constraint.go index ae4e6ee364..0d02665c0d 100644 --- a/coderd/database/foreign_key_constraint.go +++ b/coderd/database/foreign_key_constraint.go @@ -19,7 +19,6 @@ const ( ForeignKeyChatMessagesChatID ForeignKeyConstraint = "chat_messages_chat_id_fkey" // ALTER TABLE ONLY chat_messages ADD CONSTRAINT chat_messages_chat_id_fkey FOREIGN KEY (chat_id) REFERENCES chats(id) ON DELETE CASCADE; ForeignKeyChatMessagesModelConfigID ForeignKeyConstraint = "chat_messages_model_config_id_fkey" // ALTER TABLE ONLY chat_messages ADD CONSTRAINT chat_messages_model_config_id_fkey FOREIGN KEY (model_config_id) REFERENCES chat_model_configs(id); ForeignKeyChatModelConfigsCreatedBy ForeignKeyConstraint = "chat_model_configs_created_by_fkey" // ALTER TABLE ONLY chat_model_configs ADD CONSTRAINT chat_model_configs_created_by_fkey FOREIGN KEY (created_by) REFERENCES users(id); - ForeignKeyChatModelConfigsProvider ForeignKeyConstraint = "chat_model_configs_provider_fkey" // ALTER TABLE ONLY chat_model_configs ADD CONSTRAINT chat_model_configs_provider_fkey FOREIGN KEY (provider) REFERENCES chat_providers(provider) ON DELETE CASCADE; ForeignKeyChatModelConfigsUpdatedBy ForeignKeyConstraint = "chat_model_configs_updated_by_fkey" // ALTER TABLE ONLY chat_model_configs ADD CONSTRAINT chat_model_configs_updated_by_fkey FOREIGN KEY (updated_by) REFERENCES users(id); ForeignKeyChatProvidersAPIKeyKeyID ForeignKeyConstraint = "chat_providers_api_key_key_id_fkey" // ALTER TABLE ONLY chat_providers ADD CONSTRAINT chat_providers_api_key_key_id_fkey FOREIGN KEY (api_key_key_id) REFERENCES dbcrypt_keys(active_key_digest); ForeignKeyChatProvidersCreatedBy ForeignKeyConstraint = "chat_providers_created_by_fkey" // ALTER TABLE ONLY chat_providers ADD CONSTRAINT chat_providers_created_by_fkey FOREIGN KEY (created_by) REFERENCES users(id); diff --git a/coderd/database/migrations/000474_drop_chat_model_config_provider_fk.down.sql b/coderd/database/migrations/000474_drop_chat_model_config_provider_fk.down.sql new file mode 100644 index 0000000000..3b5ce550ce --- /dev/null +++ b/coderd/database/migrations/000474_drop_chat_model_config_provider_fk.down.sql @@ -0,0 +1,27 @@ +-- Restore placeholder provider rows before re-adding the provider FK. +-- +-- The companion up migration dropped chat_model_configs.provider's foreign +-- key, so historical model-config rows can outlive a deleted provider row. +-- These backfilled providers are deliberately disabled stubs with empty +-- credential fields, which lets rollback restore referential integrity +-- without re-enabling a provider. This insert depends on the current +-- provider whitelist still admitting every historical +-- chat_model_configs.provider value, and on the omitted columns keeping +-- compatible defaults. Operators restoring a real provider should update the +-- stub row, including credential-policy flags such as +-- central_api_key_enabled, before enabling it, rather than insert a second +-- row with the same provider name. +INSERT INTO chat_providers (provider, enabled) +SELECT DISTINCT + cmc.provider, + FALSE +FROM + chat_model_configs cmc +LEFT JOIN + chat_providers cp ON cp.provider = cmc.provider +WHERE + cp.provider IS NULL; + +ALTER TABLE chat_model_configs + ADD CONSTRAINT chat_model_configs_provider_fkey + FOREIGN KEY (provider) REFERENCES chat_providers(provider) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000474_drop_chat_model_config_provider_fk.up.sql b/coderd/database/migrations/000474_drop_chat_model_config_provider_fk.up.sql new file mode 100644 index 0000000000..385eeb8a2c --- /dev/null +++ b/coderd/database/migrations/000474_drop_chat_model_config_provider_fk.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE chat_model_configs + DROP CONSTRAINT chat_model_configs_provider_fkey; diff --git a/coderd/database/querier.go b/coderd/database/querier.go index a6d279cd16..34858374c8 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -113,6 +113,7 @@ type sqlcQuerier interface { // archive-cleanup retry). DeleteChatDebugDataByChatID(ctx context.Context, arg DeleteChatDebugDataByChatIDParams) (int64, error) DeleteChatModelConfigByID(ctx context.Context, id uuid.UUID) error + DeleteChatModelConfigsByProvider(ctx context.Context, provider string) error DeleteChatProviderByID(ctx context.Context, id uuid.UUID) error DeleteChatQueuedMessage(ctx context.Context, arg DeleteChatQueuedMessageParams) error DeleteChatUsageLimitGroupOverride(ctx context.Context, groupID uuid.UUID) error @@ -327,7 +328,9 @@ type sqlcQuerier interface { GetChatModelConfigsForTelemetry(ctx context.Context) ([]GetChatModelConfigsForTelemetryRow, error) GetChatPlanModeInstructions(ctx context.Context) (string, error) GetChatProviderByID(ctx context.Context, id uuid.UUID) (ChatProvider, error) + GetChatProviderByIDForUpdate(ctx context.Context, id uuid.UUID) (ChatProvider, error) GetChatProviderByProvider(ctx context.Context, provider string) (ChatProvider, error) + GetChatProviderByProviderForUpdate(ctx context.Context, provider string) (ChatProvider, error) GetChatProviders(ctx context.Context) ([]ChatProvider, error) GetChatQueuedMessages(ctx context.Context, chatID uuid.UUID) ([]ChatQueuedMessage, error) // Returns the chat retention period in days. Chats archived longer diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 3edb997c82..a9a27f265b 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -4283,6 +4283,23 @@ func (q *sqlQuerier) DeleteChatModelConfigByID(ctx context.Context, id uuid.UUID return err } +const deleteChatModelConfigsByProvider = `-- name: DeleteChatModelConfigsByProvider :exec +UPDATE + chat_model_configs +SET + deleted = TRUE, + deleted_at = NOW(), + updated_at = NOW() +WHERE + provider = $1::text + AND deleted = FALSE +` + +func (q *sqlQuerier) DeleteChatModelConfigsByProvider(ctx context.Context, provider string) error { + _, err := q.db.ExecContext(ctx, deleteChatModelConfigsByProvider, provider) + return err +} + const getChatModelConfigByID = `-- name: GetChatModelConfigByID :one SELECT id, provider, model, display_name, created_by, updated_by, enabled, is_default, deleted, deleted_at, created_at, updated_at, context_limit, compression_threshold, options @@ -4699,6 +4716,37 @@ func (q *sqlQuerier) GetChatProviderByID(ctx context.Context, id uuid.UUID) (Cha return i, err } +const getChatProviderByIDForUpdate = `-- name: GetChatProviderByIDForUpdate :one +SELECT + id, provider, display_name, api_key, api_key_key_id, created_by, enabled, created_at, updated_at, base_url, central_api_key_enabled, allow_user_api_key, allow_central_api_key_fallback +FROM + chat_providers +WHERE + id = $1::uuid +FOR UPDATE +` + +func (q *sqlQuerier) GetChatProviderByIDForUpdate(ctx context.Context, id uuid.UUID) (ChatProvider, error) { + row := q.db.QueryRowContext(ctx, getChatProviderByIDForUpdate, id) + var i ChatProvider + err := row.Scan( + &i.ID, + &i.Provider, + &i.DisplayName, + &i.APIKey, + &i.ApiKeyKeyID, + &i.CreatedBy, + &i.Enabled, + &i.CreatedAt, + &i.UpdatedAt, + &i.BaseUrl, + &i.CentralApiKeyEnabled, + &i.AllowUserApiKey, + &i.AllowCentralApiKeyFallback, + ) + return i, err +} + const getChatProviderByProvider = `-- name: GetChatProviderByProvider :one SELECT id, provider, display_name, api_key, api_key_key_id, created_by, enabled, created_at, updated_at, base_url, central_api_key_enabled, allow_user_api_key, allow_central_api_key_fallback @@ -4729,6 +4777,37 @@ func (q *sqlQuerier) GetChatProviderByProvider(ctx context.Context, provider str return i, err } +const getChatProviderByProviderForUpdate = `-- name: GetChatProviderByProviderForUpdate :one +SELECT + id, provider, display_name, api_key, api_key_key_id, created_by, enabled, created_at, updated_at, base_url, central_api_key_enabled, allow_user_api_key, allow_central_api_key_fallback +FROM + chat_providers +WHERE + provider = $1::text +FOR UPDATE +` + +func (q *sqlQuerier) GetChatProviderByProviderForUpdate(ctx context.Context, provider string) (ChatProvider, error) { + row := q.db.QueryRowContext(ctx, getChatProviderByProviderForUpdate, provider) + var i ChatProvider + err := row.Scan( + &i.ID, + &i.Provider, + &i.DisplayName, + &i.APIKey, + &i.ApiKeyKeyID, + &i.CreatedBy, + &i.Enabled, + &i.CreatedAt, + &i.UpdatedAt, + &i.BaseUrl, + &i.CentralApiKeyEnabled, + &i.AllowUserApiKey, + &i.AllowCentralApiKeyFallback, + ) + return i, err +} + const getChatProviders = `-- name: GetChatProviders :many SELECT id, provider, display_name, api_key, api_key_key_id, created_by, enabled, created_at, updated_at, base_url, central_api_key_enabled, allow_user_api_key, allow_central_api_key_fallback diff --git a/coderd/database/queries/chatmodelconfigs.sql b/coderd/database/queries/chatmodelconfigs.sql index 1463b96cf3..d129760c3d 100644 --- a/coderd/database/queries/chatmodelconfigs.sql +++ b/coderd/database/queries/chatmodelconfigs.sql @@ -127,3 +127,14 @@ SET updated_at = NOW() WHERE id = @id::uuid; + +-- name: DeleteChatModelConfigsByProvider :exec +UPDATE + chat_model_configs +SET + deleted = TRUE, + deleted_at = NOW(), + updated_at = NOW() +WHERE + provider = @provider::text + AND deleted = FALSE; diff --git a/coderd/database/queries/chatproviders.sql b/coderd/database/queries/chatproviders.sql index 02edac049d..7df983541d 100644 --- a/coderd/database/queries/chatproviders.sql +++ b/coderd/database/queries/chatproviders.sql @@ -6,6 +6,15 @@ FROM WHERE id = @id::uuid; +-- name: GetChatProviderByIDForUpdate :one +SELECT + * +FROM + chat_providers +WHERE + id = @id::uuid +FOR UPDATE; + -- name: GetChatProviderByProvider :one SELECT * @@ -14,6 +23,15 @@ FROM WHERE provider = @provider::text; +-- name: GetChatProviderByProviderForUpdate :one +SELECT + * +FROM + chat_providers +WHERE + provider = @provider::text +FOR UPDATE; + -- name: GetChatProviders :many SELECT * diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index a4205851db..3433402ef9 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -5386,27 +5386,29 @@ func (api *API) deleteChatProvider(rw http.ResponseWriter, r *http.Request) { return } - if _, err := api.Database.GetChatProviderByID(ctx, providerID); err != nil { - if httpapi.Is404Error(err) { - httpapi.ResourceNotFound(rw) - return + err := api.Database.InTx(func(tx database.Store) error { + provider, err := tx.GetChatProviderByIDForUpdate(ctx, providerID) + switch { + case err == nil: + if err := tx.DeleteChatModelConfigsByProvider(ctx, provider.Provider); err != nil { + return xerrors.Errorf("soft delete chat model configs for provider %q: %w", provider.Provider, err) + } + if err := ensureDefaultChatModelConfig(ctx, tx); err != nil { + return err + } + if err := tx.DeleteChatProviderByID(ctx, provider.ID); err != nil { + return xerrors.Errorf("delete chat provider %s: %w", provider.ID, err) + } + return nil + case xerrors.Is(err, sql.ErrNoRows): + return err + default: + return xerrors.Errorf("get chat provider %s for delete: %w", providerID, err) } - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to get chat provider.", - Detail: err.Error(), - }) - return - } - - if err := api.Database.DeleteChatProviderByID(ctx, providerID); err != nil { - if database.IsForeignKeyViolation(err, - database.ForeignKeyChatMessagesModelConfigID, - database.ForeignKeyChatsLastModelConfigID, - ) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Provider models are still referenced by existing chats.", - Detail: err.Error(), - }) + }, nil) + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + httpapi.ResourceNotFound(rw) return } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -5700,6 +5702,10 @@ func (api *API) createChatModelConfig(rw http.ResponseWriter, r *http.Request) { var inserted database.ChatModelConfig err := api.Database.InTx(func(tx database.Store) error { + if err := requireChatProviderForModelConfig(ctx, tx, insertParams.Provider); err != nil { + return err + } + insertAsDefault := isDefault if !insertAsDefault { _, err := tx.GetDefaultChatModelConfig(ctx) @@ -5745,7 +5751,7 @@ func (api *API) createChatModelConfig(rw http.ResponseWriter, r *http.Request) { Detail: err.Error(), }) return - case database.IsForeignKeyViolation(err): + case xerrors.Is(err, errChatProviderNotConfigured): httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Chat provider is not configured.", Detail: err.Error(), @@ -5878,6 +5884,10 @@ func (api *API) updateChatModelConfig(rw http.ResponseWriter, r *http.Request) { var updated database.ChatModelConfig err = api.Database.InTx(func(tx database.Store) error { + if err := requireChatProviderForModelConfig(ctx, tx, updateParams.Provider); err != nil { + return err + } + setAsDefault := updateParams.IsDefault && !existing.IsDefault if setAsDefault { if err := tx.UnsetDefaultChatModelConfigs(ctx); err != nil { @@ -5887,6 +5897,9 @@ func (api *API) updateChatModelConfig(rw http.ResponseWriter, r *http.Request) { _, err := tx.UpdateChatModelConfig(ctx, updateParams) if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + return errChatModelConfigNotFound + } return err } @@ -5905,6 +5918,10 @@ func (api *API) updateChatModelConfig(rw http.ResponseWriter, r *http.Request) { refreshedConfig, err := tx.GetChatModelConfigByID(ctx, existing.ID) if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + // Do not wrap with %w. The outer handler maps target misses to 404. + return xerrors.Errorf("refresh updated chat model config: %v", err) + } return xerrors.Errorf("refresh updated chat model config: %w", err) } updated = refreshedConfig @@ -5918,12 +5935,15 @@ func (api *API) updateChatModelConfig(rw http.ResponseWriter, r *http.Request) { Detail: err.Error(), }) return - case database.IsForeignKeyViolation(err): + case xerrors.Is(err, errChatProviderNotConfigured): httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Chat provider is not configured.", Detail: err.Error(), }) return + case xerrors.Is(err, errChatModelConfigNotFound): + httpapi.ResourceNotFound(rw) + return default: httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to update chat model config.", @@ -6024,6 +6044,11 @@ func ensureDefaultChatModelConfig( params := chatModelConfigToUpdateParams(candidateConfig) params.IsDefault = true if _, err := tx.UpdateChatModelConfig(ctx, params); err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + // Do not wrap with %w. Callers map target misses to 404, but a + // default-candidate race is an internal retryable failure. + return xerrors.Errorf("set default model config: %v", err) + } return xerrors.Errorf("set default model config: %w", err) } return nil @@ -6313,6 +6338,30 @@ func chatProviderValidationDetail() string { return "Provider must be one of: " + strings.Join(chatprovider.SupportedProviders(), ", ") + "." } +var ( + errChatModelConfigNotFound = xerrors.New("chat model config not found") + errChatProviderNotConfigured = xerrors.New("chat provider is not configured") +) + +// requireChatProviderForModelConfig takes a FOR UPDATE lock on the provider +// row to serialize model-config writes with deleteChatProvider. Do not swap +// this call for the non-locking provider lookup. +func requireChatProviderForModelConfig( + ctx context.Context, + tx database.Store, + provider string, +) error { + _, err := tx.GetChatProviderByProviderForUpdate(ctx, provider) + switch { + case err == nil: + return nil + case xerrors.Is(err, sql.ErrNoRows): + return errChatProviderNotConfigured + default: + return xerrors.Errorf("get chat provider %q: %w", provider, err) + } +} + const maxChatProviderAPIKeySize = 10240 // 10 KB func validateChatProviderAPIKeySize(apiKey string) error { diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index b008c0ce09..eb1c0e4feb 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -123,6 +123,44 @@ func (s *failNextChatSystemPromptStore) GetChatSystemPromptConfig(ctx context.Co return s.Store.GetChatSystemPromptConfig(ctx) } +// failNextUpdateChatModelConfigStore shares its failure state across InTx +// wrappers so tests can force a specific in-transaction model-config update to +// return sql.ErrNoRows. +type failNextUpdateChatModelConfigStore struct { + database.Store + + failNextUpdateChatModelConfig *atomic.Bool + failNextUpdateChatModelConfigID uuid.UUID +} + +func newFailNextUpdateChatModelConfigStore(store database.Store) *failNextUpdateChatModelConfigStore { + return &failNextUpdateChatModelConfigStore{ + Store: store, + failNextUpdateChatModelConfig: &atomic.Bool{}, + } +} + +func (s *failNextUpdateChatModelConfigStore) InTx(function func(database.Store) error, txOpts *database.TxOptions) error { + return s.Store.InTx(func(tx database.Store) error { + return function(&failNextUpdateChatModelConfigStore{ + Store: tx, + failNextUpdateChatModelConfig: s.failNextUpdateChatModelConfig, + failNextUpdateChatModelConfigID: s.failNextUpdateChatModelConfigID, + }) + }, txOpts) +} + +func (s *failNextUpdateChatModelConfigStore) UpdateChatModelConfig( + ctx context.Context, + arg database.UpdateChatModelConfigParams, +) (database.ChatModelConfig, error) { + if arg.ID == s.failNextUpdateChatModelConfigID && + s.failNextUpdateChatModelConfig.CompareAndSwap(true, false) { + return database.ChatModelConfig{}, sql.ErrNoRows + } + return s.Store.UpdateChatModelConfig(ctx, arg) +} + func requireChatUsageLimitExceededError( t *testing.T, err error, @@ -2532,6 +2570,202 @@ func TestDeleteChatProvider(t *testing.T) { } }) + t.Run("SuccessWithHistoricalChats", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + + providerToDelete, err := client.CreateChatProvider(ctx, codersdk.CreateChatProviderConfigRequest{ + Provider: "openai", + APIKey: "delete-api-key", + AllowUserAPIKey: ptr.Ref(true), + }) + require.NoError(t, err) + + deleteContextLimit := int64(4096) + deleteIsDefault := true + configToDelete, err := client.CreateChatModelConfig(ctx, codersdk.CreateChatModelConfigRequest{ + Provider: providerToDelete.Provider, + Model: "gpt-4o-delete-provider", + ContextLimit: &deleteContextLimit, + IsDefault: &deleteIsDefault, + }) + require.NoError(t, err) + + keepProvider, err := client.CreateChatProvider(ctx, codersdk.CreateChatProviderConfigRequest{ + Provider: "anthropic", + APIKey: "keep-api-key", + }) + require.NoError(t, err) + + keepContextLimit := int64(8192) + keepConfig, err := client.CreateChatModelConfig(ctx, codersdk.CreateChatModelConfigRequest{ + Provider: keepProvider.Provider, + Model: "claude-keep-provider", + ContextLimit: &keepContextLimit, + }) + require.NoError(t, err) + + chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + ModelConfigID: ptr.Ref(configToDelete.ID), + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "provider delete history " + t.Name(), + }}, + }) + require.NoError(t, err) + require.Equal(t, configToDelete.ID, chat.LastModelConfigID) + + insertAssistantCostMessage(ctx, t, db, chat.ID, configToDelete.ID, 500) + + _, err = client.UpsertUserChatProviderKey(ctx, providerToDelete.ID, codersdk.CreateUserChatProviderKeyRequest{ + APIKey: "user-delete-key", + }) + require.NoError(t, err) + + userKeys, err := db.GetUserChatProviderKeys(dbauthz.AsSystemRestricted(ctx), firstUser.UserID) + require.NoError(t, err) + require.Len(t, userKeys, 1) + require.Equal(t, providerToDelete.ID, userKeys[0].ChatProviderID) + + err = client.DeleteChatProvider(ctx, providerToDelete.ID) + require.NoError(t, err) + + _, err = db.GetChatProviderByID(dbauthz.AsSystemRestricted(ctx), providerToDelete.ID) + require.ErrorIs(t, err, sql.ErrNoRows) + + providers, err := client.ListChatProviders(ctx) + require.NoError(t, err) + foundKeepProvider := false + for _, listed := range providers { + require.NotEqual(t, providerToDelete.ID, listed.ID) + if listed.ID == keepProvider.ID { + foundKeepProvider = true + } + } + require.True(t, foundKeepProvider) + + configs, err := client.ListChatModelConfigs(ctx) + require.NoError(t, err) + foundDeletedConfig := false + foundKeepConfig := false + for _, config := range configs { + if config.ID == configToDelete.ID { + foundDeletedConfig = true + } + if config.ID == keepConfig.ID { + foundKeepConfig = true + require.True(t, config.IsDefault) + } + } + require.False(t, foundDeletedConfig) + require.True(t, foundKeepConfig) + + defaultConfig, err := db.GetDefaultChatModelConfig(dbauthz.AsSystemRestricted(ctx)) + require.NoError(t, err) + require.Equal(t, keepConfig.ID, defaultConfig.ID) + + _, err = db.GetChatModelConfigByID(dbauthz.AsSystemRestricted(ctx), configToDelete.ID) + require.ErrorIs(t, err, sql.ErrNoRows) + + gotChat, err := client.GetChat(ctx, chat.ID) + require.NoError(t, err) + require.Equal(t, chat.ID, gotChat.ID) + require.Equal(t, configToDelete.ID, gotChat.LastModelConfigID) + + messages, err := client.GetChatMessages(ctx, chat.ID, nil) + require.NoError(t, err) + foundHistoricalMessage := false + for _, message := range messages.Messages { + if message.ModelConfigID != nil && *message.ModelConfigID == configToDelete.ID { + foundHistoricalMessage = true + break + } + } + require.True(t, foundHistoricalMessage) + + userKeys, err = db.GetUserChatProviderKeys(dbauthz.AsSystemRestricted(ctx), firstUser.UserID) + require.NoError(t, err) + require.Empty(t, userKeys) + }) + + t.Run("SuccessWithHistoricalChatsAndNoReplacementConfig", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + + provider, err := client.CreateChatProvider(ctx, codersdk.CreateChatProviderConfigRequest{ + Provider: "openai", + APIKey: "only-provider-api-key", + }) + require.NoError(t, err) + + contextLimit := int64(4096) + isDefault := true + config, err := client.CreateChatModelConfig(ctx, codersdk.CreateChatModelConfigRequest{ + Provider: provider.Provider, + Model: "gpt-4o-only-provider", + ContextLimit: &contextLimit, + IsDefault: &isDefault, + }) + require.NoError(t, err) + + chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + ModelConfigID: ptr.Ref(config.ID), + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "only provider delete history " + t.Name(), + }}, + }) + require.NoError(t, err) + require.Equal(t, config.ID, chat.LastModelConfigID) + + insertAssistantCostMessage(ctx, t, db, chat.ID, config.ID, 250) + + err = client.DeleteChatProvider(ctx, provider.ID) + require.NoError(t, err) + + providers, err := client.ListChatProviders(ctx) + require.NoError(t, err) + for _, listed := range providers { + require.NotEqual(t, provider.ID, listed.ID) + } + + _, err = db.GetChatProviderByID(dbauthz.AsSystemRestricted(ctx), provider.ID) + require.ErrorIs(t, err, sql.ErrNoRows) + + _, err = db.GetChatModelConfigByID(dbauthz.AsSystemRestricted(ctx), config.ID) + require.ErrorIs(t, err, sql.ErrNoRows) + + _, err = db.GetDefaultChatModelConfig(dbauthz.AsSystemRestricted(ctx)) + require.ErrorIs(t, err, sql.ErrNoRows) + + configs, err := client.ListChatModelConfigs(ctx) + require.NoError(t, err) + require.Empty(t, configs) + + gotChat, err := client.GetChat(ctx, chat.ID) + require.NoError(t, err) + require.Equal(t, config.ID, gotChat.LastModelConfigID) + + messages, err := client.GetChatMessages(ctx, chat.ID, nil) + require.NoError(t, err) + foundHistoricalMessage := false + for _, message := range messages.Messages { + if message.ModelConfigID != nil && *message.ModelConfigID == config.ID { + foundHistoricalMessage = true + break + } + } + require.True(t, foundHistoricalMessage) + }) + t.Run("NotFound", func(t *testing.T) { t.Parallel() @@ -3506,6 +3740,84 @@ func TestUpdateChatModelConfig(t *testing.T) { ) }) + t.Run("ProviderNotConfigured", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + _ = coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createChatModelConfig(t, client) + + _, err := client.UpdateChatModelConfig(ctx, modelConfig.ID, codersdk.UpdateChatModelConfigRequest{ + Provider: "anthropic", + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "Chat provider is not configured.", sdkErr.Message) + }) + + t.Run("NotFoundWhenTargetRowDisappearsInTx", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + rawDB, pubsub := dbtestutil.NewDB(t) + store := newFailNextUpdateChatModelConfigStore(rawDB) + client := codersdk.NewExperimentalClient(coderdtest.New(t, &coderdtest.Options{ + Database: store, + Pubsub: pubsub, + DeploymentValues: chatDeploymentValues(t), + })) + _ = coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createChatModelConfig(t, client) + + store.failNextUpdateChatModelConfigID = modelConfig.ID + store.failNextUpdateChatModelConfig.Store(true) + + _, err := client.UpdateChatModelConfig(ctx, modelConfig.ID, codersdk.UpdateChatModelConfigRequest{ + DisplayName: "missing in tx", + }) + requireSDKError(t, err, http.StatusNotFound) + }) + + t.Run("InternalServerErrorWhenDefaultCandidateDisappearsInTx", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + rawDB, pubsub := dbtestutil.NewDB(t) + store := newFailNextUpdateChatModelConfigStore(rawDB) + client := codersdk.NewExperimentalClient(coderdtest.New(t, &coderdtest.Options{ + Database: store, + Pubsub: pubsub, + DeploymentValues: chatDeploymentValues(t), + })) + _ = coderdtest.CreateFirstUser(t, client.Client) + defaultConfig := createChatModelConfig(t, client) + + _, err := client.CreateChatProvider(ctx, codersdk.CreateChatProviderConfigRequest{ + Provider: "anthropic", + APIKey: "candidate-api-key", + }) + require.NoError(t, err) + + contextLimit := int64(4096) + isDefault := false + candidateConfig, err := client.CreateChatModelConfig(ctx, codersdk.CreateChatModelConfigRequest{ + Provider: "anthropic", + Model: "claude-3-5-sonnet", + ContextLimit: &contextLimit, + IsDefault: &isDefault, + }) + require.NoError(t, err) + + store.failNextUpdateChatModelConfigID = candidateConfig.ID + store.failNextUpdateChatModelConfig.Store(true) + + _, err = client.UpdateChatModelConfig(ctx, defaultConfig.ID, codersdk.UpdateChatModelConfigRequest{ + IsDefault: ptr.Ref(false), + }) + sdkErr := requireSDKError(t, err, http.StatusInternalServerError) + require.Equal(t, "Failed to update chat model config.", sdkErr.Message) + }) + t.Run("NotFound", func(t *testing.T) { t.Parallel()