diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index e78055c42a..15c97794e8 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -7258,17 +7258,6 @@ func (q *querier) UpdateChatStatus(ctx context.Context, arg database.UpdateChatS return q.db.UpdateChatStatus(ctx, arg) } -func (q *querier) UpdateChatStatusPreserveUpdatedAt(ctx context.Context, arg database.UpdateChatStatusPreserveUpdatedAtParams) (database.Chat, error) { - chat, err := q.db.GetChatByID(ctx, arg.ID) - if err != nil { - return database.Chat{}, err - } - if err := q.authorizeContext(ctx, policy.ActionUpdate, chat); err != nil { - return database.Chat{}, err - } - return q.db.UpdateChatStatusPreserveUpdatedAt(ctx, arg) -} - func (q *querier) UpdateChatTitleByID(ctx context.Context, arg database.UpdateChatTitleByIDParams) (database.Chat, error) { chat, err := q.db.GetChatByID(ctx, arg.ID) if err != nil { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 3ec4aabfb2..1696b3f3c4 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1462,16 +1462,6 @@ func (s *MethodTestSuite) TestChats() { dbm.EXPECT().UpdateChatPlanModeByID(gomock.Any(), arg).Return(chat, nil).AnyTimes() check.Args(arg).Asserts(chat, policy.ActionUpdate).Returns(chat) })) - s.Run("UpdateChatStatusPreserveUpdatedAt", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { - chat := testutil.Fake(s.T(), faker, database.Chat{}) - arg := database.UpdateChatStatusPreserveUpdatedAtParams{ - ID: chat.ID, - Status: database.ChatStatusRunning, - } - dbm.EXPECT().GetChatByID(gomock.Any(), chat.ID).Return(chat, nil).AnyTimes() - dbm.EXPECT().UpdateChatStatusPreserveUpdatedAt(gomock.Any(), arg).Return(chat, nil).AnyTimes() - check.Args(arg).Asserts(chat, policy.ActionUpdate).Returns(chat) - })) s.Run("UpdateChatHeartbeats", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { resultID := uuid.New() arg := database.UpdateChatHeartbeatsParams{ diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index d31796e73b..e2afe293ac 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -5209,14 +5209,6 @@ func (m queryMetricsStore) UpdateChatStatus(ctx context.Context, arg database.Up return r0, r1 } -func (m queryMetricsStore) UpdateChatStatusPreserveUpdatedAt(ctx context.Context, arg database.UpdateChatStatusPreserveUpdatedAtParams) (database.Chat, error) { - start := time.Now() - r0, r1 := m.s.UpdateChatStatusPreserveUpdatedAt(ctx, arg) - m.queryLatencies.WithLabelValues("UpdateChatStatusPreserveUpdatedAt").Observe(time.Since(start).Seconds()) - m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "UpdateChatStatusPreserveUpdatedAt").Inc() - return r0, r1 -} - func (m queryMetricsStore) UpdateChatTitleByID(ctx context.Context, arg database.UpdateChatTitleByIDParams) (database.Chat, error) { start := time.Now() r0, r1 := m.s.UpdateChatTitleByID(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 466c5c36f4..3c3c6eb816 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -9815,21 +9815,6 @@ func (mr *MockStoreMockRecorder) UpdateChatStatus(ctx, arg any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateChatStatus", reflect.TypeOf((*MockStore)(nil).UpdateChatStatus), ctx, arg) } -// UpdateChatStatusPreserveUpdatedAt mocks base method. -func (m *MockStore) UpdateChatStatusPreserveUpdatedAt(ctx context.Context, arg database.UpdateChatStatusPreserveUpdatedAtParams) (database.Chat, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateChatStatusPreserveUpdatedAt", ctx, arg) - ret0, _ := ret[0].(database.Chat) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// UpdateChatStatusPreserveUpdatedAt indicates an expected call of UpdateChatStatusPreserveUpdatedAt. -func (mr *MockStoreMockRecorder) UpdateChatStatusPreserveUpdatedAt(ctx, arg any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateChatStatusPreserveUpdatedAt", reflect.TypeOf((*MockStore)(nil).UpdateChatStatusPreserveUpdatedAt), ctx, arg) -} - // UpdateChatTitleByID mocks base method. func (m *MockStore) UpdateChatTitleByID(ctx context.Context, arg database.UpdateChatTitleByIDParams) (database.Chat, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4aab48dde1..b6ef3537ac 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1373,7 +1373,6 @@ type sqlcQuerier interface { // assigned by trigger from the current snapshot_version. UpdateChatRetryState(ctx context.Context, arg UpdateChatRetryStateParams) (Chat, error) UpdateChatStatus(ctx context.Context, arg UpdateChatStatusParams) (Chat, error) - UpdateChatStatusPreserveUpdatedAt(ctx context.Context, arg UpdateChatStatusPreserveUpdatedAtParams) (Chat, error) UpdateChatTitleByID(ctx context.Context, arg UpdateChatTitleByIDParams) (Chat, error) UpdateChatWorkspaceBinding(ctx context.Context, arg UpdateChatWorkspaceBindingParams) (Chat, error) UpdateCryptoKeyDeletesAt(ctx context.Context, arg UpdateCryptoKeyDeletesAtParams) (CryptoKey, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 587aaafcfc..1cd81d3b82 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -12762,11 +12762,11 @@ func TestUpdateChatLastTurnSummary(t *testing.T) { require.NoError(t, err) require.EqualValues(t, 1, affected) - advancedUpdatedAt := chat.UpdatedAt.Add(time.Second) - _, err = db.UpdateChatStatusPreserveUpdatedAt(ctx, database.UpdateChatStatusPreserveUpdatedAtParams{ - ID: chat.ID, - Status: database.ChatStatusRunning, - UpdatedAt: advancedUpdatedAt, + // Advance updated_at with a title write so the next assertion can + // prove the summary update preserves the stored value. + advanced, err := db.UpdateChatByID(ctx, database.UpdateChatByIDParams{ + ID: chat.ID, + Title: "summary-chat-advanced", }) require.NoError(t, err) @@ -12781,7 +12781,7 @@ func TestUpdateChatLastTurnSummary(t *testing.T) { fetched, err = db.GetChatByID(ctx, chat.ID) require.NoError(t, err) require.Equal(t, sql.NullString{String: "still fresh summary", Valid: true}, fetched.LastTurnSummary) - require.Equal(t, advancedUpdatedAt, fetched.UpdatedAt) + require.Equal(t, advanced.UpdatedAt, fetched.UpdatedAt) _, err = db.LockChatAndBumpSnapshotVersion(ctx, chat.ID) require.NoError(t, err) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 0d0e40857d..0553300172 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -12141,144 +12141,6 @@ func (q *sqlQuerier) UpdateChatStatus(ctx context.Context, arg UpdateChatStatusP return i, err } -const updateChatStatusPreserveUpdatedAt = `-- name: UpdateChatStatusPreserveUpdatedAt :one -WITH updated_chat AS ( -UPDATE - chats -SET - status = $1::chat_status, - worker_id = $2::uuid, - started_at = $3::timestamptz, - heartbeat_at = $4::timestamptz, - last_error = $5::jsonb, - updated_at = $6::timestamptz -WHERE - id = $7::uuid -RETURNING id, owner_id, workspace_id, title, status, worker_id, started_at, heartbeat_at, created_at, updated_at, parent_chat_id, root_chat_id, last_model_config_id, archived, last_error, mode, mcp_server_ids, labels, build_id, agent_id, pin_order, last_read_message_id, dynamic_tools, organization_id, plan_mode, client_type, last_turn_summary, user_acl, group_acl, snapshot_version, history_version, queue_version, generation_attempt, retry_state, retry_state_version, runner_id, requires_action_deadline_at, context_aggregate_hash, context_dirty_since, context_dirty_resources, context_error -), -chats_expanded AS ( - SELECT - updated_chat.id, - updated_chat.owner_id, - updated_chat.workspace_id, - updated_chat.title, - updated_chat.status, - updated_chat.worker_id, - updated_chat.started_at, - updated_chat.heartbeat_at, - updated_chat.created_at, - updated_chat.updated_at, - updated_chat.parent_chat_id, - updated_chat.root_chat_id, - updated_chat.last_model_config_id, - updated_chat.archived, - updated_chat.last_error, - updated_chat.mode, - updated_chat.mcp_server_ids, - updated_chat.labels, - updated_chat.build_id, - updated_chat.agent_id, - updated_chat.pin_order, - updated_chat.last_read_message_id, - updated_chat.dynamic_tools, - updated_chat.organization_id, - updated_chat.plan_mode, - updated_chat.client_type, - updated_chat.last_turn_summary, - updated_chat.snapshot_version, - updated_chat.history_version, - updated_chat.queue_version, - updated_chat.generation_attempt, - updated_chat.retry_state, - updated_chat.retry_state_version, - updated_chat.runner_id, - updated_chat.requires_action_deadline_at, - COALESCE(root.user_acl, updated_chat.user_acl) AS user_acl, - COALESCE(root.group_acl, updated_chat.group_acl) AS group_acl, - owner.username AS owner_username, - owner.name AS owner_name, - updated_chat.context_aggregate_hash, - updated_chat.context_dirty_since, - updated_chat.context_dirty_resources, - updated_chat.context_error - FROM - updated_chat - LEFT JOIN chats root ON root.id = COALESCE(updated_chat.root_chat_id, updated_chat.parent_chat_id) - JOIN visible_users owner ON owner.id = updated_chat.owner_id -) -SELECT id, owner_id, workspace_id, title, status, worker_id, started_at, heartbeat_at, created_at, updated_at, parent_chat_id, root_chat_id, last_model_config_id, archived, last_error, mode, mcp_server_ids, labels, build_id, agent_id, pin_order, last_read_message_id, dynamic_tools, organization_id, plan_mode, client_type, last_turn_summary, snapshot_version, history_version, queue_version, generation_attempt, retry_state, retry_state_version, runner_id, requires_action_deadline_at, user_acl, group_acl, owner_username, owner_name, context_aggregate_hash, context_dirty_since, context_dirty_resources, context_error -FROM chats_expanded -` - -type UpdateChatStatusPreserveUpdatedAtParams struct { - Status ChatStatus `db:"status" json:"status"` - WorkerID uuid.NullUUID `db:"worker_id" json:"worker_id"` - StartedAt sql.NullTime `db:"started_at" json:"started_at"` - HeartbeatAt sql.NullTime `db:"heartbeat_at" json:"heartbeat_at"` - LastError pqtype.NullRawMessage `db:"last_error" json:"last_error"` - UpdatedAt time.Time `db:"updated_at" json:"updated_at"` - ID uuid.UUID `db:"id" json:"id"` -} - -func (q *sqlQuerier) UpdateChatStatusPreserveUpdatedAt(ctx context.Context, arg UpdateChatStatusPreserveUpdatedAtParams) (Chat, error) { - row := q.db.QueryRowContext(ctx, updateChatStatusPreserveUpdatedAt, - arg.Status, - arg.WorkerID, - arg.StartedAt, - arg.HeartbeatAt, - arg.LastError, - arg.UpdatedAt, - arg.ID, - ) - var i Chat - err := row.Scan( - &i.ID, - &i.OwnerID, - &i.WorkspaceID, - &i.Title, - &i.Status, - &i.WorkerID, - &i.StartedAt, - &i.HeartbeatAt, - &i.CreatedAt, - &i.UpdatedAt, - &i.ParentChatID, - &i.RootChatID, - &i.LastModelConfigID, - &i.Archived, - &i.LastError, - &i.Mode, - pq.Array(&i.MCPServerIDs), - &i.Labels, - &i.BuildID, - &i.AgentID, - &i.PinOrder, - &i.LastReadMessageID, - &i.DynamicTools, - &i.OrganizationID, - &i.PlanMode, - &i.ClientType, - &i.LastTurnSummary, - &i.SnapshotVersion, - &i.HistoryVersion, - &i.QueueVersion, - &i.GenerationAttempt, - &i.RetryState, - &i.RetryStateVersion, - &i.RunnerID, - &i.RequiresActionDeadlineAt, - &i.UserACL, - &i.GroupACL, - &i.OwnerUsername, - &i.OwnerName, - &i.ContextAggregateHash, - &i.ContextDirtySince, - &i.ContextDirtyResources, - &i.ContextError, - ) - return i, err -} - const updateChatTitleByID = `-- name: UpdateChatTitleByID :one WITH updated_chat AS ( UPDATE diff --git a/coderd/database/queries/chats.sql b/coderd/database/queries/chats.sql index 8d4feb773b..f6b0aba7d4 100644 --- a/coderd/database/queries/chats.sql +++ b/coderd/database/queries/chats.sql @@ -1684,74 +1684,6 @@ chats_expanded AS ( SELECT * FROM chats_expanded; --- name: UpdateChatStatusPreserveUpdatedAt :one -WITH updated_chat AS ( -UPDATE - chats -SET - status = @status::chat_status, - worker_id = sqlc.narg('worker_id')::uuid, - started_at = sqlc.narg('started_at')::timestamptz, - heartbeat_at = sqlc.narg('heartbeat_at')::timestamptz, - last_error = sqlc.narg('last_error')::jsonb, - updated_at = @updated_at::timestamptz -WHERE - id = @id::uuid -RETURNING * -), -chats_expanded AS ( - SELECT - updated_chat.id, - updated_chat.owner_id, - updated_chat.workspace_id, - updated_chat.title, - updated_chat.status, - updated_chat.worker_id, - updated_chat.started_at, - updated_chat.heartbeat_at, - updated_chat.created_at, - updated_chat.updated_at, - updated_chat.parent_chat_id, - updated_chat.root_chat_id, - updated_chat.last_model_config_id, - updated_chat.archived, - updated_chat.last_error, - updated_chat.mode, - updated_chat.mcp_server_ids, - updated_chat.labels, - updated_chat.build_id, - updated_chat.agent_id, - updated_chat.pin_order, - updated_chat.last_read_message_id, - updated_chat.dynamic_tools, - updated_chat.organization_id, - updated_chat.plan_mode, - updated_chat.client_type, - updated_chat.last_turn_summary, - updated_chat.snapshot_version, - updated_chat.history_version, - updated_chat.queue_version, - updated_chat.generation_attempt, - updated_chat.retry_state, - updated_chat.retry_state_version, - updated_chat.runner_id, - updated_chat.requires_action_deadline_at, - COALESCE(root.user_acl, updated_chat.user_acl) AS user_acl, - COALESCE(root.group_acl, updated_chat.group_acl) AS group_acl, - owner.username AS owner_username, - owner.name AS owner_name, - updated_chat.context_aggregate_hash, - updated_chat.context_dirty_since, - updated_chat.context_dirty_resources, - updated_chat.context_error - FROM - updated_chat - LEFT JOIN chats root ON root.id = COALESCE(updated_chat.root_chat_id, updated_chat.parent_chat_id) - JOIN visible_users owner ON owner.id = updated_chat.owner_id -) -SELECT * -FROM chats_expanded; - -- name: GetStaleChats :many -- Find chats that appear stuck and need recovery: -- 1. Running chats whose heartbeat has expired (worker crash). diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index d491eeb742..83c95c1578 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -2625,12 +2625,6 @@ func (api *API) applyChatTitleUpdate( updatedChat, wrote, err := api.chatDaemon.RenameChatTitle(ctx, chat, trimmedTitle) if err != nil { - if errors.Is(err, chatd.ErrManualTitleRegenerationInProgress) { - httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ - Message: "Title regeneration already in progress for this chat.", - }) - return chat, true - } if errors.Is(err, sql.ErrNoRows) { httpapi.ResourceNotFound(rw) return chat, true @@ -3838,9 +3832,9 @@ func (api *API) regenerateChatTitle(rw http.ResponseWriter, r *http.Request) { ctx = aibridge.WithDelegatedAPIKeyID(ctx, apiKey.ID) updatedChat, err := api.chatDaemon.RegenerateChatTitle(ctx, chat) if err != nil { - if errors.Is(err, chatd.ErrManualTitleRegenerationInProgress) { - httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ - Message: "Title regeneration already in progress for this chat.", + if errors.Is(err, chatd.ErrNoDefaultChatModelConfig) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "No default chat model config is configured.", }) return } @@ -3888,9 +3882,9 @@ func (api *API) proposeChatTitle(rw http.ResponseWriter, r *http.Request) { ctx = aibridge.WithDelegatedAPIKeyID(ctx, apiKey.ID) title, err := api.chatDaemon.ProposeChatTitle(ctx, chat) if err != nil { - if errors.Is(err, chatd.ErrManualTitleRegenerationInProgress) { - httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ - Message: "Title regeneration already in progress for this chat.", + if errors.Is(err, chatd.ErrNoDefaultChatModelConfig) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "No default chat model config is configured.", }) return } diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index ac326a1514..d79e2f83e9 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -8575,53 +8575,13 @@ func TestRegenerateChatTitle(t *testing.T) { ) }) - t.Run("AlreadyInProgress", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitLong) - client, db := newChatClientWithDatabase(t) - user := coderdtest.CreateFirstUser(t, client.Client) - modelConfig := createChatModelConfig(t, client) - - chat := dbgen.Chat(t, db, database.Chat{ - OrganizationID: user.OrganizationID, - OwnerID: user.UserID, - LastModelConfigID: modelConfig.ID, - Title: "chat with lock held", - }) - - _, err := db.UpdateChatStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateChatStatusParams{ - ID: chat.ID, - Status: database.ChatStatusCompleted, - WorkerID: uuid.NullUUID{UUID: uuid.MustParse("00000000-0000-0000-0000-000000000001"), Valid: true}, - StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, - HeartbeatAt: sql.NullTime{Time: time.Now(), Valid: true}, - LastError: pqtype.NullRawMessage{}, - }) - require.NoError(t, err) - - res, err := client.Request( - ctx, - http.MethodPost, - fmt.Sprintf("/api/experimental/chats/%s/title/regenerate", chat.ID), - nil, - ) - require.NoError(t, err) - defer res.Body.Close() - require.Equal(t, http.StatusConflict, res.StatusCode) - - var resp codersdk.Response - require.NoError(t, json.NewDecoder(res.Body).Decode(&resp)) - require.Equal(t, "Title regeneration already in progress for this chat.", resp.Message) - }) - t.Run("PendingWithoutWorker", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) client, db := newChatClientWithDatabase(t) user := coderdtest.CreateFirstUser(t, client.Client) - modelConfig := createChatModelConfig(t, client) + modelConfig := createTitleGenerationModelConfig(t, client) chat := dbgen.Chat(t, db, database.Chat{ OrganizationID: user.OrganizationID, @@ -8629,6 +8589,7 @@ func TestRegenerateChatTitle(t *testing.T) { LastModelConfigID: modelConfig.ID, Title: "pending chat without worker", }) + seedManualTitleSourceMessage(t, db, chat, modelConfig.ID) var err error chat, err = db.UpdateChatStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateChatStatusParams{ @@ -8641,28 +8602,31 @@ func TestRegenerateChatTitle(t *testing.T) { }) require.NoError(t, err) - before, err := db.GetChatByID(dbauthz.AsSystemRestricted(ctx), chat.ID) + // Pending chats are never acquired + // (GetChatWorkerAcquisitionCandidates excludes the status), so + // manual title regeneration must still proceed. + updated, err := client.RegenerateChatTitle(ctx, chat.ID) require.NoError(t, err) - - res, err := client.Request( - ctx, - http.MethodPost, - fmt.Sprintf("/api/experimental/chats/%s/title/regenerate", chat.ID), - nil, - ) - require.NoError(t, err) - defer res.Body.Close() - require.Equal(t, http.StatusConflict, res.StatusCode) - - var resp codersdk.Response - require.NoError(t, json.NewDecoder(res.Body).Decode(&resp)) - require.Equal(t, "Title regeneration already in progress for this chat.", resp.Message) + require.Equal(t, "Test Chat", updated.Title) persisted, err := db.GetChatByID(dbauthz.AsSystemRestricted(ctx), chat.ID) require.NoError(t, err) + require.Equal(t, "Test Chat", persisted.Title) require.Equal(t, database.ChatStatusPending, persisted.Status) require.False(t, persisted.WorkerID.Valid) - require.True(t, persisted.UpdatedAt.Equal(before.UpdatedAt)) + }) + + t.Run("NoDefaultModelConfig", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + chat := seedChatWithDeletedModelConfig(ctx, t, db, user) + + _, err := client.RegenerateChatTitle(ctx, chat.ID) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "No default chat model config is configured.", sdkErr.Message) }) t.Run("RegenerationFailure", func(t *testing.T) { @@ -8755,6 +8719,122 @@ func TestProposeChatTitle(t *testing.T) { requireSDKError(t, err, http.StatusNotFound) }) + t.Run("Unauthenticated", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + _ = createChatModelConfig(t, client) + + chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "chat for unauthenticated proposal", + }}, + }) + require.NoError(t, err) + + unauthenticatedClient := codersdk.NewExperimentalClient(codersdk.New(client.URL)) + _, err = unauthenticatedClient.ProposeChatTitle(ctx, chat.ID) + requireSDKError(t, err, http.StatusUnauthorized) + }) + + t.Run("PendingWithoutWorker", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createTitleGenerationModelConfig(t, client) + + chat := dbgen.Chat(t, db, database.Chat{ + OrganizationID: user.OrganizationID, + OwnerID: user.UserID, + LastModelConfigID: modelConfig.ID, + Title: "pending chat without worker", + }) + seedManualTitleSourceMessage(t, db, chat, modelConfig.ID) + + var err error + chat, err = db.UpdateChatStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateChatStatusParams{ + ID: chat.ID, + Status: database.ChatStatusPending, + WorkerID: uuid.NullUUID{}, + StartedAt: sql.NullTime{}, + HeartbeatAt: sql.NullTime{}, + LastError: pqtype.NullRawMessage{}, + }) + require.NoError(t, err) + + before, err := db.GetChatByID(dbauthz.AsSystemRestricted(ctx), chat.ID) + require.NoError(t, err) + + // Pending chats are never acquired + // (GetChatWorkerAcquisitionCandidates excludes the status), so + // title proposal must still proceed. + resp, err := client.ProposeChatTitle(ctx, chat.ID) + require.NoError(t, err) + require.Equal(t, "Test Chat", resp.Title) + + persisted, err := db.GetChatByID(dbauthz.AsSystemRestricted(ctx), chat.ID) + require.NoError(t, err) + require.Equal(t, before.Title, persisted.Title, + "propose must not persist the suggested title") + require.Equal(t, database.ChatStatusPending, persisted.Status) + require.False(t, persisted.WorkerID.Valid) + require.True(t, persisted.UpdatedAt.Equal(before.UpdatedAt)) + }) + + t.Run("NoDefaultModelConfig", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + chat := seedChatWithDeletedModelConfig(ctx, t, db, user) + + _, err := client.ProposeChatTitle(ctx, chat.ID) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "No default chat model config is configured.", sdkErr.Message) + }) + + t.Run("StoppedWorkspace", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createTitleGenerationModelConfig(t, client) + + workspaceBuild := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OrganizationID: user.OrganizationID, + OwnerID: user.UserID, + }).WithAgent().Do() + dbfake.WorkspaceBuild(t, db, workspaceBuild.Workspace).Seed(database.WorkspaceBuild{ + Transition: database.WorkspaceTransitionStop, + BuildNumber: 2, + }).Do() + + // Chats bound to stopped workspaces settle in waiting (or + // error). Title generation never touches the workspace, so it + // must still succeed. + chat := dbgen.Chat(t, db, database.Chat{ + OrganizationID: user.OrganizationID, + OwnerID: user.UserID, + LastModelConfigID: modelConfig.ID, + WorkspaceID: uuid.NullUUID{UUID: workspaceBuild.Workspace.ID, Valid: true}, + Status: database.ChatStatusWaiting, + Title: "stopped workspace chat", + }) + seedManualTitleSourceMessage(t, db, chat, modelConfig.ID) + + resp, err := client.ProposeChatTitle(ctx, chat.ID) + require.NoError(t, err) + require.Equal(t, "Test Chat", resp.Title) + }) + t.Run("DoesNotPersistTitleOrBumpUpdatedAt", func(t *testing.T) { t.Parallel() @@ -10884,6 +10964,67 @@ func aiProviderBaseURLForTest(provider string) string { } } +// seedManualTitleSourceMessage inserts a visible user message so manual +// title generation has content to summarize. +func seedManualTitleSourceMessage( + t testing.TB, + db database.Store, + chat database.Chat, + modelConfigID uuid.UUID, +) { + t.Helper() + + content, err := chatprompt.MarshalParts([]codersdk.ChatMessagePart{ + codersdk.ChatMessageText("manual title source"), + }) + require.NoError(t, err) + _ = dbgen.ChatMessage(t, db, database.ChatMessage{ + ChatID: chat.ID, + CreatedBy: uuid.NullUUID{UUID: chat.OwnerID, Valid: true}, + ModelConfigID: uuid.NullUUID{UUID: modelConfigID, Valid: true}, + Role: database.ChatMessageRoleUser, + Visibility: database.ChatMessageVisibilityBoth, + Content: content, + }) +} + +// createTitleGenerationModelConfig provisions a model config on the openai +// provider type, which routes structured title generation through the +// Responses API. The chattest fake answers it with {"title": "Test Chat"}. +func createTitleGenerationModelConfig( + t *testing.T, + client *codersdk.ExperimentalClient, +) codersdk.ChatModelConfig { + t.Helper() + return createAdditionalChatModelConfig(t, client, "openai", "gpt-4.1") +} + +// seedChatWithDeletedModelConfig creates a chat whose only model config is +// soft-deleted, leaving the deployment without a usable model config. The +// config exists only to satisfy the chats foreign key. +func seedChatWithDeletedModelConfig( + ctx context.Context, + t *testing.T, + db database.Store, + user codersdk.CreateFirstUserResponse, +) database.Chat { + t.Helper() + + modelConfig := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{}) + chat := dbgen.Chat(t, db, database.Chat{ + OrganizationID: user.OrganizationID, + OwnerID: user.UserID, + LastModelConfigID: modelConfig.ID, + Title: "chat without model config", + }) + seedManualTitleSourceMessage(t, db, chat, modelConfig.ID) + require.NoError(t, db.DeleteChatModelConfigByID( + dbauthz.AsSystemRestricted(ctx), + modelConfig.ID, + )) + return chat +} + func createChatModelConfig(t testing.TB, client *codersdk.ExperimentalClient) codersdk.ChatModelConfig { t.Helper() return coderdtest.CreateOpenAICompatChatModelConfig(t, client, "") diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 5c85a2968f..f57058f136 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -1094,6 +1094,9 @@ var ( // accept modifications (messages, edits, promotions, or // tool-result submissions). ErrChatArchived = xerrors.New("chat is archived") + // ErrNoDefaultChatModelConfig indicates no default chat model config + // is configured, so chatd cannot resolve a model for the request. + ErrNoDefaultChatModelConfig = xerrors.New("no default chat model config is configured") ) // UsageLimitExceededError indicates the user has exceeded their chat spend @@ -1560,7 +1563,7 @@ func resolveFallbackModelConfigID( defaultConfig, err := store.GetDefaultChatModelConfig(chatdCtx) if err != nil { if errors.Is(err, sql.ErrNoRows) { - return uuid.Nil, xerrors.New("no default chat model config is available") + return uuid.Nil, ErrNoDefaultChatModelConfig } return uuid.Nil, xerrors.Errorf("get default chat model config: %w", err) } @@ -2106,10 +2109,6 @@ func (p *Server) ReconcileInvalidStateChat( const manualTitleMessageWindowLimit = 50 -var ErrManualTitleRegenerationInProgress = xerrors.New( - "manual title regeneration already in progress", -) - type manualTitleCandidateResult struct { title string modelConfig database.ChatModelConfig @@ -2166,117 +2165,6 @@ func (e *manualTitleGenerationError) Unwrap() error { return e.cause } -var manualTitleLockWorkerID = uuid.MustParse( - "00000000-0000-0000-0000-000000000001", -) - -const manualTitleLockStaleAfter = time.Minute - -func isFreshManualTitleLock(chat database.Chat, now time.Time) bool { - if !chat.WorkerID.Valid || chat.WorkerID.UUID != manualTitleLockWorkerID { - return false - } - leaseAt := chat.HeartbeatAt - if !leaseAt.Valid { - leaseAt = chat.StartedAt - } - return leaseAt.Valid && leaseAt.Time.After(now.Add(-manualTitleLockStaleAfter)) -} - -// updateChatStatusPreserveUpdatedAt applies internal lock transitions without -// changing chat recency, because chat list ordering uses updated_at. -func updateChatStatusPreserveUpdatedAt( - ctx context.Context, - store database.Store, - chat database.Chat, - workerID uuid.NullUUID, - startedAt sql.NullTime, - heartbeatAt sql.NullTime, -) (database.Chat, error) { - return store.UpdateChatStatusPreserveUpdatedAt( - ctx, - database.UpdateChatStatusPreserveUpdatedAtParams{ - ID: chat.ID, - Status: chat.Status, - WorkerID: workerID, - StartedAt: startedAt, - HeartbeatAt: heartbeatAt, - LastError: chat.LastError, - UpdatedAt: chat.UpdatedAt, - }, - ) -} - -func (p *Server) acquireManualTitleLock(ctx context.Context, chatID uuid.UUID) error { - now := time.Now() - return p.db.InTx(func(tx database.Store) error { - lockedChat, err := tx.GetChatByIDForUpdate(ctx, chatID) - if err != nil { - return xerrors.Errorf("lock chat for manual title regeneration: %w", err) - } - // Only a fresh manual lock or a chat without a real worker should - // block title regeneration. Running chats with a real worker may - // regenerate their title concurrently, and last write wins. - hasRealWorker := lockedChat.Status == database.ChatStatusRunning && - lockedChat.WorkerID.Valid && - lockedChat.WorkerID.UUID != manualTitleLockWorkerID - if lockedChat.Status == database.ChatStatusPending || - (lockedChat.Status == database.ChatStatusRunning && !hasRealWorker) || - isFreshManualTitleLock(lockedChat, now) { - return ErrManualTitleRegenerationInProgress - } - if hasRealWorker { - return nil - } - - _, err = updateChatStatusPreserveUpdatedAt( - ctx, - tx, - lockedChat, - uuid.NullUUID{UUID: manualTitleLockWorkerID, Valid: true}, - sql.NullTime{Time: now, Valid: true}, - sql.NullTime{}, - ) - if err != nil { - return xerrors.Errorf("mark chat for manual title regeneration: %w", err) - } - return nil - }, database.DefaultTXOptions().WithID("chat_title_regenerate_lock")) -} - -func (p *Server) releaseManualTitleLock(ctx context.Context, chatID uuid.UUID) { - cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - - err := p.db.InTx(func(tx database.Store) error { - lockedChat, err := tx.GetChatByIDForUpdate(cleanupCtx, chatID) - if err != nil { - return xerrors.Errorf("lock chat to release manual title regeneration: %w", err) - } - if !lockedChat.WorkerID.Valid || lockedChat.WorkerID.UUID != manualTitleLockWorkerID { - return nil - } - _, err = updateChatStatusPreserveUpdatedAt( - cleanupCtx, - tx, - lockedChat, - uuid.NullUUID{}, - sql.NullTime{}, - sql.NullTime{}, - ) - if err != nil { - return xerrors.Errorf("clear manual title regeneration marker: %w", err) - } - return nil - }, database.DefaultTXOptions().WithID("chat_title_regenerate_unlock")) - if err != nil { - p.logger.Warn(cleanupCtx, "failed to release manual title regeneration marker", - slog.F("chat_id", chatID), - slog.Error(err), - ) - } -} - // RegenerateChatTitle regenerates a chat title from the chat's visible // messages, persists it when it changes, and broadcasts the update. func (p *Server) RegenerateChatTitle( @@ -2287,11 +2175,6 @@ func (p *Server) RegenerateChatTitle( // keeping chat ownership authorization at the HTTP layer. //nolint:gocritic // Non-admin users need chatd-scoped config reads here. chatdCtx := dbauthz.AsChatd(ctx) - if err := p.acquireManualTitleLock(ctx, chat.ID); err != nil { - return database.Chat{}, err - } - defer p.releaseManualTitleLock(chatdCtx, chat.ID) - updatedChat, err := p.regenerateChatTitleWithStore( chatdCtx, p.db, @@ -2309,13 +2192,6 @@ func (p *Server) RenameChatTitle( chat database.Chat, newTitle string, ) (updated database.Chat, wrote bool, err error) { - //nolint:gocritic // Lock release needs chatd-scoped writes. - chatdCtx := dbauthz.AsChatd(ctx) - if err := p.acquireManualTitleLock(ctx, chat.ID); err != nil { - return database.Chat{}, false, err - } - defer p.releaseManualTitleLock(chatdCtx, chat.ID) - currentChat, err := p.db.GetChatByID(ctx, chat.ID) if err != nil { return database.Chat{}, false, xerrors.Errorf("get chat for rename: %w", err) @@ -2339,18 +2215,14 @@ func (p *Server) PublishTitleChange(chat database.Chat) { p.publishChatPubsubEvent(chat, codersdk.ChatWatchEventKindTitleChange, nil) } -// ProposeChatTitle generates a title suggestion from the chat's visible messages without persisting it. +// ProposeChatTitle generates a title suggestion from the chat's +// visible messages without persisting it. func (p *Server) ProposeChatTitle( ctx context.Context, chat database.Chat, ) (string, error) { //nolint:gocritic // Non-admin users need chatd-scoped config reads here. chatdCtx := dbauthz.AsChatd(ctx) - if err := p.acquireManualTitleLock(ctx, chat.ID); err != nil { - return "", err - } - defer p.releaseManualTitleLock(chatdCtx, chat.ID) - title, err := p.proposeChatTitleWithStore(chatdCtx, p.db, chat) if err != nil { return "", p.recordManualTitleGenerationFailure(ctx, chat, err) @@ -2374,7 +2246,7 @@ func (p *Server) recordManualTitleGenerationFailure( 5*time.Second, ) defer recordCancel() - if _, recordErr := recordManualTitleUsage( + if _, _, recordErr := recordManualTitleUsage( recordCtx, p.db, chat, @@ -2499,7 +2371,7 @@ func (p *Server) proposeChatTitleWithStore( recordCtx, recordCancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) defer recordCancel() - if _, recordErr := recordManualTitleUsage( + if _, _, recordErr := recordManualTitleUsage( recordCtx, store, chat, @@ -2529,7 +2401,7 @@ func (p *Server) regenerateChatTitleWithStore( recordCtx, recordCancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) defer recordCancel() - updatedChat, recordErr := recordManualTitleUsage( + updatedChat, wroteTitle, recordErr := recordManualTitleUsage( recordCtx, store, chat, @@ -2544,7 +2416,11 @@ func (p *Server) regenerateChatTitleWithStore( } return database.Chat{}, xerrors.Errorf("record manual title usage: %w", recordErr) } - if updatedChat.Title == chat.Title { + // Publish only when this regeneration wrote the title. When a + // concurrent rename won the race, the rename path already published + // the fresher title; re-publishing the re-read row here could + // deliver a stale title_change after an even newer rename. + if !wroteTitle { return updatedChat, nil } @@ -2868,6 +2744,12 @@ func fantasyUsageToChatMessageUsage(usage fantasy.Usage) codersdk.ChatMessageUsa return chatUsage } +// recordManualTitleUsage stores token accounting for a manual title +// generation and, when newTitle is set, persists it only if the chat +// title still matches the caller's snapshot. The returned bool reports +// whether the title was actually written; it is false when newTitle is +// empty, when a concurrent writer changed the title first, or when +// newTitle matches the current title. func recordManualTitleUsage( ctx context.Context, store database.Store, @@ -2876,10 +2758,10 @@ func recordManualTitleUsage( usage fantasy.Usage, activeAPIKeyID string, newTitle string, -) (database.Chat, error) { +) (database.Chat, bool, error) { hasUsage := usage != (fantasy.Usage{}) if !hasUsage && newTitle == "" { - return chat, nil + return chat, false, nil } var totalCostMicros *int64 @@ -2887,7 +2769,7 @@ func recordManualTitleUsage( callConfig := codersdk.ChatModelCallConfig{} if len(modelConfig.Options) > 0 { if err := json.Unmarshal(modelConfig.Options, &callConfig); err != nil { - return database.Chat{}, xerrors.Errorf("parse model call config: %w", err) + return database.Chat{}, false, xerrors.Errorf("parse model call config: %w", err) } } totalCostMicros = chatcost.CalculateTotalCostMicros( @@ -2903,12 +2785,14 @@ func recordManualTitleUsage( content := "[]" updatedChat := chat + wroteTitle := false err := store.InTx(func(tx database.Store) error { lockedChat, err := tx.GetChatByIDForUpdate(ctx, chat.ID) if err != nil { return xerrors.Errorf("lock chat for manual title usage: %w", err) } updatedChat = lockedChat + wroteTitle = false if hasUsage { messages, err := tx.InsertChatMessages(ctx, database.InsertChatMessagesParams{ ChatID: chat.ID, @@ -2956,13 +2840,14 @@ func recordManualTitleUsage( if err != nil { return xerrors.Errorf("update chat title: %w", err) } + wroteTitle = true } return nil }, nil) if err != nil { - return database.Chat{}, err + return database.Chat{}, false, err } - return updatedChat, nil + return updatedChat, wroteTitle, nil } type chatMessage struct { @@ -4434,9 +4319,7 @@ func (p *Server) resolveModelConfig( defaultConfig, err := p.configCache.DefaultModelConfig(ctx) if err != nil { if xerrors.Is(err, sql.ErrNoRows) { - return database.ChatModelConfig{}, xerrors.New( - "no default chat model config is available", - ) + return database.ChatModelConfig{}, ErrNoDefaultChatModelConfig } return database.ChatModelConfig{}, xerrors.Errorf( "get default chat model config: %w", err, diff --git a/coderd/x/chatd/chatd_internal_test.go b/coderd/x/chatd/chatd_internal_test.go index 4e1a41e084..930e395fdc 100644 --- a/coderd/x/chatd/chatd_internal_test.go +++ b/coderd/x/chatd/chatd_internal_test.go @@ -676,29 +676,6 @@ func TestStopAfterBehaviorTools(t *testing.T) { func TestRenameChatTitle(t *testing.T) { t.Parallel() - setupRealWorkerLock := func( - db *dbmock.MockStore, - chatID uuid.UUID, - lockedChat database.Chat, - ) { - lockTx := dbmock.NewMockStore(gomock.NewController(t)) - unlockTx := dbmock.NewMockStore(gomock.NewController(t)) - gomock.InOrder( - db.EXPECT().InTx(gomock.Any(), database.DefaultTXOptions().WithID("chat_title_regenerate_lock")).DoAndReturn( - func(fn func(database.Store) error, _ *database.TxOptions) error { - return fn(lockTx) - }, - ), - db.EXPECT().InTx(gomock.Any(), database.DefaultTXOptions().WithID("chat_title_regenerate_unlock")).DoAndReturn( - func(fn func(database.Store) error, _ *database.TxOptions) error { - return fn(unlockTx) - }, - ), - ) - lockTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(lockedChat, nil) - unlockTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(lockedChat, nil) - } - t.Run("WritesAndReturnsWroteTrue", func(t *testing.T) { t.Parallel() @@ -720,7 +697,6 @@ func TestRenameChatTitle(t *testing.T) { server := &Server{db: db, logger: logger} - setupRealWorkerLock(db, chatID, stored) db.EXPECT().GetChatByID(gomock.Any(), chatID).Return(stored, nil) db.EXPECT().UpdateChatTitleByID(gomock.Any(), database.UpdateChatTitleByIDParams{ ID: chatID, @@ -754,7 +730,6 @@ func TestRenameChatTitle(t *testing.T) { server := &Server{db: db, logger: logger} - setupRealWorkerLock(db, chatID, landed) db.EXPECT().GetChatByID(gomock.Any(), chatID).Return(landed, nil) got, wrote, err := server.RenameChatTitle(ctx, stale, "landed-concurrently") @@ -793,9 +768,7 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) ctrl := gomock.NewController(t) db := dbmock.NewMockStore(ctrl) - lockTx := dbmock.NewMockStore(ctrl) usageTx := dbmock.NewMockStore(ctrl) - unlockTx := dbmock.NewMockStore(ctrl) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) pubsub := dbpubsub.NewInMemory() clock := quartz.NewReal() @@ -860,6 +833,7 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts(t *testing.T) { db: db, logger: logger, pubsub: pubsub, + clock: quartz.NewReal(), configCache: newChatConfigCache(context.Background(), db, clock), aibridgeTransportFactory: aibridgeTestFactoryPointer(factory), } @@ -913,29 +887,13 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts(t *testing.T) { db.EXPECT().GetChatTitleGenerationModelOverride(gomock.Any()).Return("", nil) db.EXPECT().GetEnabledChatModelConfigs(gomock.Any()).Return(nil, nil) - gomock.InOrder( - db.EXPECT().InTx(gomock.Any(), database.DefaultTXOptions().WithID("chat_title_regenerate_lock")).DoAndReturn( - func(fn func(database.Store) error, opts *database.TxOptions) error { - require.Equal(t, "chat_title_regenerate_lock", opts.TxIdentifier) - return fn(lockTx) - }, - ), - db.EXPECT().InTx(gomock.Any(), nil).DoAndReturn( - func(fn func(database.Store) error, opts *database.TxOptions) error { - require.Nil(t, opts) - return fn(usageTx) - }, - ), - db.EXPECT().InTx(gomock.Any(), database.DefaultTXOptions().WithID("chat_title_regenerate_unlock")).DoAndReturn( - func(fn func(database.Store) error, opts *database.TxOptions) error { - require.Equal(t, "chat_title_regenerate_unlock", opts.TxIdentifier) - return fn(unlockTx) - }, - ), + db.EXPECT().InTx(gomock.Any(), nil).DoAndReturn( + func(fn func(database.Store) error, opts *database.TxOptions) error { + require.Nil(t, opts) + return fn(usageTx) + }, ) - lockTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(chat, nil) - usageTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(chat, nil) usageTx.EXPECT().InsertChatMessages(gomock.Any(), gomock.AssignableToTypeOf(database.InsertChatMessagesParams{})).DoAndReturn( func(_ context.Context, arg database.InsertChatMessagesParams) ([]database.ChatMessage, error) { @@ -951,8 +909,6 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts(t *testing.T) { Title: wantTitle, }).Return(updatedChat, nil) - unlockTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(updatedChat, nil) - gotChat, err := server.RegenerateChatTitle(ctx, chat) require.NoError(t, err) require.Equal(t, updatedChat, gotChat) @@ -968,15 +924,19 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts(t *testing.T) { } } -func TestRegenerateChatTitle_PersistsAndBroadcasts_IdleChatReleasesManualLock(t *testing.T) { +// With no request-level locking, recordManualTitleUsage's re-read under +// GetChatByIDForUpdate is the only protection against clobbering a title +// that changed while the model call ran. The strict mock has no +// UpdateChatByID expectation, so any persist attempt fails the test. +// A skipped persist must also not publish a title_change event; the +// wroteTitle comment in regenerateChatTitleWithStore explains why. +func TestRegenerateChatTitle_SkipsPersistWhenTitleChangedConcurrently(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) ctrl := gomock.NewController(t) db := dbmock.NewMockStore(ctrl) - lockTx := dbmock.NewMockStore(ctrl) usageTx := dbmock.NewMockStore(ctrl) - unlockTx := dbmock.NewMockStore(ctrl) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) pubsub := dbpubsub.NewInMemory() clock := quartz.NewReal() @@ -987,51 +947,40 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts_IdleChatReleasesManualLock(t providerID := uuid.New() userPrompt := "review pull request 23633 and fix review threads" activeAPIKeyID := "key-" + uuid.NewString() - wantTitle := "Review PR 23633" + generatedTitle := "Review PR 23633" chat := database.Chat{ ID: chatID, OwnerID: ownerID, LastModelConfigID: modelConfigID, - Status: database.ChatStatusCompleted, + Status: database.ChatStatusWaiting, Title: fallbackChatTitle(userPrompt), } - lockedChat := chat - lockedChat.WorkerID = uuid.NullUUID{UUID: manualTitleLockWorkerID, Valid: true} - lockedChat.StartedAt = sql.NullTime{Time: time.Now(), Valid: true} modelConfig := database.ChatModelConfig{ ID: modelConfigID, Model: "gpt-4o-mini", ContextLimit: 8192, AIProviderID: uuid.NullUUID{UUID: providerID, Valid: true}, } - updatedChat := lockedChat - updatedChat.Title = wantTitle - unlockedChat := updatedChat - unlockedChat.WorkerID = uuid.NullUUID{} - unlockedChat.StartedAt = sql.NullTime{} + // Another writer (rename or a second regenerate) landed while the + // model call was in flight. + landedChat := chat + landedChat.Title = "landed-concurrently" - messageEvents := make(chan struct { - payload codersdk.ChatWatchEvent - err error - }, 1) + titleEvents := make(chan codersdk.ChatWatchEvent, 1) cancelSub, err := pubsub.SubscribeWithErr( coderdpubsub.ChatWatchEventChannel(ownerID), coderdpubsub.HandleChatWatchEvent(func(_ context.Context, payload codersdk.ChatWatchEvent, err error) { - messageEvents <- struct { - payload codersdk.ChatWatchEvent - err error - }{payload: payload, err: err} + require.NoError(t, err) + titleEvents <- payload }), ) require.NoError(t, err) defer cancelSub() - // Title generation routes through the transport factory, so the model - // response is synthesized by the RoundTripper (see aibridgeTestFactory). factory := &aibridgeTestFactory{rt: roundTripFunc(func(req *http.Request) (*http.Response, error) { requireOutgoingRequestModel(t, req, modelConfig.Model) - text := strconv.Quote(`{"title":"` + wantTitle + `"}`) + text := strconv.Quote(`{"title":"` + generatedTitle + `"}`) body := `{"id":"resp_test","object":"response","created_at":0,"status":"completed","model":"gpt-4o-mini","output":[{"id":"msg_test","type":"message","role":"assistant","content":[{"type":"output_text","text":` + text + `}]}],"usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2}}` return &http.Response{ StatusCode: http.StatusOK, @@ -1045,6 +994,7 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts_IdleChatReleasesManualLock(t db: db, logger: logger, pubsub: pubsub, + clock: quartz.NewReal(), configCache: newChatConfigCache(context.Background(), db, clock), aibridgeTransportFactory: aibridgeTestFactoryPointer(factory), } @@ -1079,12 +1029,6 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts_IdleChatReleasesManualLock(t database.ChatMessageVisibilityBoth, codersdk.ChatMessageText(userPrompt), ), activeAPIKeyID), - mustChatMessage( - t, - database.ChatMessageRoleAssistant, - database.ChatMessageVisibilityBoth, - codersdk.ChatMessageText("checking the diff now"), - ), }, nil) db.EXPECT().GetChatMessagesByChatIDDescPaginated( gomock.Any(), @@ -1097,84 +1041,28 @@ func TestRegenerateChatTitle_PersistsAndBroadcasts_IdleChatReleasesManualLock(t db.EXPECT().GetChatTitleGenerationModelOverride(gomock.Any()).Return("", nil) db.EXPECT().GetEnabledChatModelConfigs(gomock.Any()).Return(nil, nil) - gomock.InOrder( - db.EXPECT().InTx(gomock.Any(), database.DefaultTXOptions().WithID("chat_title_regenerate_lock")).DoAndReturn( - func(fn func(database.Store) error, opts *database.TxOptions) error { - require.Equal(t, "chat_title_regenerate_lock", opts.TxIdentifier) - return fn(lockTx) - }, - ), - db.EXPECT().InTx(gomock.Any(), nil).DoAndReturn( - func(fn func(database.Store) error, opts *database.TxOptions) error { - require.Nil(t, opts) - return fn(usageTx) - }, - ), - db.EXPECT().InTx(gomock.Any(), database.DefaultTXOptions().WithID("chat_title_regenerate_unlock")).DoAndReturn( - func(fn func(database.Store) error, opts *database.TxOptions) error { - require.Equal(t, "chat_title_regenerate_unlock", opts.TxIdentifier) - return fn(unlockTx) - }, - ), - ) - - lockTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(chat, nil) - lockTx.EXPECT().UpdateChatStatusPreserveUpdatedAt( - gomock.Any(), - gomock.AssignableToTypeOf(database.UpdateChatStatusPreserveUpdatedAtParams{}), - ).DoAndReturn(func(_ context.Context, arg database.UpdateChatStatusPreserveUpdatedAtParams) (database.Chat, error) { - require.Equal(t, chat.ID, arg.ID) - require.Equal(t, chat.Status, arg.Status) - require.Equal(t, uuid.NullUUID{UUID: manualTitleLockWorkerID, Valid: true}, arg.WorkerID) - require.True(t, arg.StartedAt.Valid) - require.WithinDuration(t, time.Now(), arg.StartedAt.Time, time.Second) - require.False(t, arg.HeartbeatAt.Valid) - require.Equal(t, chat.LastError, arg.LastError) - require.Equal(t, chat.UpdatedAt, arg.UpdatedAt) - return lockedChat, nil - }) - - usageTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(lockedChat, nil) - usageTx.EXPECT().InsertChatMessages(gomock.Any(), gomock.AssignableToTypeOf(database.InsertChatMessagesParams{})).DoAndReturn( - func(_ context.Context, arg database.InsertChatMessagesParams) ([]database.ChatMessage, error) { - require.Equal(t, []uuid.UUID{ownerID}, arg.CreatedBy) - require.Equal(t, []uuid.UUID{modelConfigID}, arg.ModelConfigID) - require.Equal(t, []string{"[]"}, arg.Content) - return []database.ChatMessage{{ID: 91}}, nil + db.EXPECT().InTx(gomock.Any(), nil).DoAndReturn( + func(fn func(database.Store) error, _ *database.TxOptions) error { + return fn(usageTx) }, ) + + usageTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(landedChat, nil) + usageTx.EXPECT().InsertChatMessages(gomock.Any(), gomock.AssignableToTypeOf(database.InsertChatMessagesParams{})).Return([]database.ChatMessage{{ID: 91}}, nil) usageTx.EXPECT().SoftDeleteChatMessageByID(gomock.Any(), int64(91)).Return(nil) - usageTx.EXPECT().UpdateChatByID(gomock.Any(), database.UpdateChatByIDParams{ - ID: chatID, - Title: wantTitle, - }).Return(updatedChat, nil) - - unlockTx.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(updatedChat, nil) - unlockTx.EXPECT().UpdateChatStatusPreserveUpdatedAt( - gomock.Any(), - database.UpdateChatStatusPreserveUpdatedAtParams{ - ID: updatedChat.ID, - Status: updatedChat.Status, - WorkerID: uuid.NullUUID{}, - StartedAt: sql.NullTime{}, - HeartbeatAt: sql.NullTime{}, - LastError: updatedChat.LastError, - UpdatedAt: updatedChat.UpdatedAt, - }, - ).Return(unlockedChat, nil) gotChat, err := server.RegenerateChatTitle(ctx, chat) require.NoError(t, err) - require.Equal(t, updatedChat, gotChat) + require.Equal(t, landedChat.Title, gotChat.Title, + "the concurrently landed title must survive; the generated title must not be persisted") + // The in-memory pubsub delivers synchronously, so any event published + // during RegenerateChatTitle is already buffered by now. select { - case event := <-messageEvents: - require.NoError(t, event.err) - require.Equal(t, codersdk.ChatWatchEventKindTitleChange, event.payload.Kind) - require.Equal(t, chatID, event.payload.Chat.ID) - require.Equal(t, wantTitle, event.payload.Chat.Title) - case <-time.After(time.Second): - t.Fatal("timed out waiting for title change pubsub event") + case event := <-titleEvents: + t.Fatalf("unexpected %s event published for skipped regeneration (title %q)", + event.Kind, event.Chat.Title) + default: } } diff --git a/coderd/x/chatd/quickgen_internal_test.go b/coderd/x/chatd/quickgen_internal_test.go index 729bf506f3..f2cfef1a24 100644 --- a/coderd/x/chatd/quickgen_internal_test.go +++ b/coderd/x/chatd/quickgen_internal_test.go @@ -395,13 +395,7 @@ func TestMaybeGenerateChatTitlePreservesUpdatedAt(t *testing.T) { ClientType: database.ChatClientTypeUi, }) - expectedUpdatedAt := time.Date(2024, time.January, 2, 3, 4, 5, 0, time.UTC) - chat, err := db.UpdateChatStatusPreserveUpdatedAt(ctx, database.UpdateChatStatusPreserveUpdatedAtParams{ - ID: chat.ID, - Status: chat.Status, - UpdatedAt: expectedUpdatedAt, - }) - require.NoError(t, err) + expectedUpdatedAt := chat.UpdatedAt const wantTitle = "Failed workspace logs" model := &chattest.FakeModel{ diff --git a/site/src/pages/AgentsPage/components/ChatsSidebar/ChatsSidebar.stories.tsx b/site/src/pages/AgentsPage/components/ChatsSidebar/ChatsSidebar.stories.tsx index 0003af7f01..780aa7ab13 100644 --- a/site/src/pages/AgentsPage/components/ChatsSidebar/ChatsSidebar.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatsSidebar/ChatsSidebar.stories.tsx @@ -8,7 +8,7 @@ import { userChatProviderConfigsKey } from "#/api/queries/chats"; import type * as TypesGen from "#/api/typesGenerated"; import type { Chat } from "#/api/typesGenerated"; import { MockChat } from "#/testHelpers/chatEntities"; -import { MockUserOwner } from "#/testHelpers/entities"; +import { MockUserOwner, mockApiError } from "#/testHelpers/entities"; import { withAuthProvider, withDashboardProvider, @@ -1152,6 +1152,115 @@ export const RenameChatGenerateErrorSurfacesAlert: Story = { expect(alert).toHaveTextContent( "Proposal provider is temporarily unavailable.", ); + // Plain errors have no API detail, so no developer-console hint or + // second line may leak into the alert. + expect(alert).not.toHaveTextContent("developer console"); + await waitFor(() => { + expect(input).toHaveAttribute("aria-invalid", "true"); + }); + expect(input).toHaveValue("Original title"); + expect(body.getByRole("button", { name: "Generate" })).toBeEnabled(); + }, +}; + +export const RenameChatGenerateApiErrorWithoutDetailHidesHint: Story = { + args: { + chats: [ + buildChat({ + id: "rename-generate-api-error-no-detail", + title: "Original title", + updated_at: recentTimestamp, + }), + ], + onProposeTitle: fn(async () => { + throw mockApiError({ + message: "No default chat model config is configured.", + }); + }), + onRenameTitle: fn(() => Promise.resolve()), + }, + parameters: { + reactRouter: reactRouterParameters({ + location: { path: "/agents" }, + routing: agentsRouting, + }), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const body = within(document.body); + + await userEvent.click( + canvas.getByRole("button", { + name: "Open actions for Original title", + }), + ); + await userEvent.click( + await body.findByRole("menuitem", { name: "Rename chat" }), + ); + + await body.findByRole("textbox", { + name: "Chat title", + }); + + await userEvent.click(body.getByRole("button", { name: "Generate" })); + + const alert = await body.findByRole("alert"); + expect(alert).toHaveTextContent( + "No default chat model config is configured.", + ); + // An API error without a detail field must not surface the generic + // developer-console hint as a second line. + expect(alert).not.toHaveTextContent("developer console"); + }, +}; + +export const RenameChatGenerateApiErrorShowsDetail: Story = { + args: { + chats: [ + buildChat({ + id: "rename-generate-api-error", + title: "Original title", + updated_at: recentTimestamp, + }), + ], + onProposeTitle: fn(async () => { + throw mockApiError({ + message: "Failed to generate chat title.", + detail: "No default chat model config is configured.", + }); + }), + onRenameTitle: fn(() => Promise.resolve()), + }, + parameters: { + reactRouter: reactRouterParameters({ + location: { path: "/agents" }, + routing: agentsRouting, + }), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const body = within(document.body); + + await userEvent.click( + canvas.getByRole("button", { + name: "Open actions for Original title", + }), + ); + await userEvent.click( + await body.findByRole("menuitem", { name: "Rename chat" }), + ); + + const input = await body.findByRole("textbox", { + name: "Chat title", + }); + + await userEvent.click(body.getByRole("button", { name: "Generate" })); + + const alert = await body.findByRole("alert"); + expect(alert).toHaveTextContent("Failed to generate chat title."); + expect(alert).toHaveTextContent( + "No default chat model config is configured.", + ); await waitFor(() => { expect(input).toHaveAttribute("aria-invalid", "true"); }); diff --git a/site/src/pages/AgentsPage/components/ChatsSidebar/dialogs/RenameChatDialog.tsx b/site/src/pages/AgentsPage/components/ChatsSidebar/dialogs/RenameChatDialog.tsx index e17489f0ba..f9274e2da0 100644 --- a/site/src/pages/AgentsPage/components/ChatsSidebar/dialogs/RenameChatDialog.tsx +++ b/site/src/pages/AgentsPage/components/ChatsSidebar/dialogs/RenameChatDialog.tsx @@ -7,7 +7,7 @@ import { useRef, useState, } from "react"; -import { getErrorMessage } from "#/api/errors"; +import { getErrorMessage, isApiError } from "#/api/errors"; import type { Chat } from "#/api/typesGenerated"; import { Button } from "#/components/Button/Button"; import { @@ -53,9 +53,10 @@ export const RenameChatDialog: FC = ({ const [isRenamingChat, setIsRenamingChat] = useState(false); const [isGeneratingTitle, setIsGeneratingTitle] = useState(false); const [isTypingGeneratedTitle, setIsTypingGeneratedTitle] = useState(false); - const [generateTitleError, setGenerateTitleError] = useState( - null, - ); + const [generateTitleError, setGenerateTitleError] = useState<{ + message: string; + detail?: string; + } | null>(null); const inputRef = useRef(null); const generatedTitleTypingFrameRef = useRef(null); const synchronizedChatIdRef = useRef(undefined); @@ -196,9 +197,15 @@ export const RenameChatDialog: FC = ({ startGeneratedTitleTyping(newTitle, requestedSession); } catch (error) { if (sessionRef.current !== requestedSession) return; - setGenerateTitleError( - getErrorMessage(error, "Failed to generate a new title."), - ); + setGenerateTitleError({ + message: getErrorMessage(error, "Failed to generate a new title."), + // Read the response detail directly; getErrorDetail falls + // back to a generic developer-console hint for errors + // without a detail field. + detail: isApiError(error) + ? error.response.data.detail || undefined + : undefined, + }); setIsGeneratingTitle(false); } }; @@ -300,13 +307,18 @@ export const RenameChatDialog: FC = ({ aria-describedby={generateTitleError ? errorId : undefined} /> {generateTitleError && ( - +

{generateTitleError.message}

+ {generateTitleError.detail && ( +

+ {generateTitleError.detail} +

+ )} + )}