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
This commit is contained in:
Ethan
2026-04-22 19:34:34 +10:00
committed by GitHub
parent 360e119b43
commit ad1906589d
14 changed files with 628 additions and 26 deletions
+21
View File
@@ -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
+16
View File
@@ -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{})
+24
View File
@@ -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)
+44
View File
@@ -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()
-3
View File
@@ -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);
@@ -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);
@@ -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;
@@ -0,0 +1,2 @@
ALTER TABLE chat_model_configs
DROP CONSTRAINT chat_model_configs_provider_fkey;
+3
View File
@@ -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
+79
View File
@@ -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
@@ -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;
+18
View File
@@ -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
*
+71 -22
View File
@@ -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 {
+312
View File
@@ -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()