mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
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 <mafredri@gmail.com>
This commit is contained in:
Michael Suchacz
co-authored by
Mathias Fredriksson
Mathias Fredriksson
parent
4f98fa1e03
commit
1eb5d579b0
@@ -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 {
|
||||
|
||||
@@ -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{
|
||||
|
||||
-8
@@ -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)
|
||||
|
||||
Generated
-15
@@ -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()
|
||||
|
||||
Generated
-1
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
Generated
-138
@@ -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
|
||||
|
||||
@@ -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).
|
||||
|
||||
+6
-12
@@ -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
|
||||
}
|
||||
|
||||
+198
-57
@@ -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, "")
|
||||
|
||||
+29
-146
@@ -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,
|
||||
|
||||
@@ -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:
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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{
|
||||
|
||||
@@ -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<HTMLInputElement>("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<HTMLInputElement>("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");
|
||||
});
|
||||
|
||||
@@ -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<RenameChatDialogProps> = ({
|
||||
const [isRenamingChat, setIsRenamingChat] = useState(false);
|
||||
const [isGeneratingTitle, setIsGeneratingTitle] = useState(false);
|
||||
const [isTypingGeneratedTitle, setIsTypingGeneratedTitle] = useState(false);
|
||||
const [generateTitleError, setGenerateTitleError] = useState<string | null>(
|
||||
null,
|
||||
);
|
||||
const [generateTitleError, setGenerateTitleError] = useState<{
|
||||
message: string;
|
||||
detail?: string;
|
||||
} | null>(null);
|
||||
const inputRef = useRef<HTMLInputElement | null>(null);
|
||||
const generatedTitleTypingFrameRef = useRef<number | null>(null);
|
||||
const synchronizedChatIdRef = useRef<string | null | undefined>(undefined);
|
||||
@@ -196,9 +197,15 @@ export const RenameChatDialog: FC<RenameChatDialogProps> = ({
|
||||
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<RenameChatDialogProps> = ({
|
||||
aria-describedby={generateTitleError ? errorId : undefined}
|
||||
/>
|
||||
{generateTitleError && (
|
||||
<p
|
||||
<div
|
||||
id={errorId}
|
||||
role="alert"
|
||||
className="m-0 text-xs text-content-destructive"
|
||||
className="m-0 space-y-0.5 text-xs text-content-destructive"
|
||||
>
|
||||
{generateTitleError}
|
||||
</p>
|
||||
<p className="m-0">{generateTitleError.message}</p>
|
||||
{generateTitleError.detail && (
|
||||
<p className="m-0 text-content-secondary">
|
||||
{generateTitleError.detail}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
<DialogFooter className="gap-2 sm:space-x-0">
|
||||
|
||||
Reference in New Issue
Block a user