From 1eb5d579b0dc7e020a420b186ef0f4636e85661d Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 7 Jul 2026 01:09:08 +0200 Subject: [PATCH] fix: unblock manual chat title generation for unowned chats (#26963) ## Problem The Generate button in the chat Rename dialog (POST `/api/experimental/chats/{chat}/title/propose`) could fail in ways unrelated to actual concurrent title generation: - The manual title lock returned 409 for any `pending` chat and any `running` chat without a worker. Legacy `pending` rows are never acquired by workers, so those chats 409'd forever. Running chats are unowned in the normal window between message submission and worker acquisition (indefinitely when runners are down), producing spurious 409s. - A missing default chat model config surfaced as a generic 500, and the dialog hid the actionable cause carried in the error detail. ## Fix Backend (`coderd/x/chatd`, `coderd`, `coderd/database`): - Remove the manual title lock entirely. Races between title writers are already resolved by `recordManualTitleUsage`, which re-reads the chat under `GetChatByIDForUpdate` and only persists the generated title when it is unchanged since the request snapshot, so concurrent regenerates and renames settle by last write wins. The lock only suppressed duplicate model calls (the dialog already disables the button in flight, and usage limits bound spend), and its synthetic `worker_id` marker was the source of the spurious 409s. The 409 responses, the marker and staleness handling, and the now-unused `UpdateChatStatusPreserveUpdatedAt` query are gone. - New `ErrNoDefaultChatModelConfig` sentinel mapped to 400 "No default chat model config is configured." in both title endpoints, matching the POST `/chats` precedent. Frontend (`site`): - The Rename dialog error alert now renders the API error detail under the message, reading `error.response.data.detail` directly so detail-less API errors do not show the generic developer-console hint. - Removed the dead regenerate-title UI plumbing (`onRegenerateTitle` outlet wiring and the `regeneratingTitleChatIds` spinner pipeline). The Rename dialog propose flow is the only live title-generation UX; the endpoint, codersdk methods, and the `api.ts`/`queries/chats.ts` layer are kept for API consumers. ## Tests - chatd internal: a strict-mock test pinning the compare-and-swap guard (a concurrently changed title must not be clobbered by a generated one), plus the existing persist-and-broadcast coverage without lock transactions. - HTTP: `PendingWithoutWorker` expects 200 for both endpoints, `NoDefaultModelConfig` (400) subtests, a stopped-workspace propose regression, and an `Unauthenticated` propose subtest. - Storybook: stories asserting the API error detail renders in the dialog alert, and that detail-less API errors and plain errors do not leak the developer-console hint. > Authored by Mux on Mike's behalf. --------- Co-authored-by: Mathias Fredriksson --- coderd/database/dbauthz/dbauthz.go | 11 - coderd/database/dbauthz/dbauthz_test.go | 10 - coderd/database/dbmetrics/querymetrics.go | 8 - coderd/database/dbmock/dbmock.go | 15 -- coderd/database/querier.go | 1 - coderd/database/querier_test.go | 12 +- coderd/database/queries.sql.go | 138 ---------- coderd/database/queries/chats.sql | 68 ----- coderd/exp_chats.go | 18 +- coderd/exp_chats_test.go | 255 ++++++++++++++---- coderd/x/chatd/chatd.go | 175 ++---------- coderd/x/chatd/chatd_internal_test.go | 188 +++---------- coderd/x/chatd/quickgen_internal_test.go | 8 +- .../ChatsSidebar/ChatsSidebar.stories.tsx | 111 +++++++- .../ChatsSidebar/dialogs/RenameChatDialog.tsx | 34 ++- 15 files changed, 411 insertions(+), 641 deletions(-) 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} +

+ )} + )}