feat(coderd): key chat diff statuses by ref (#29081)

Part 1 of 6.

`chat_diff_statuses` had one row per chat, so a chat could only track
one PR, and switching branches overwrote the old branch's PR data. The
primary key is now `(chat_id, git_remote_origin, git_branch)`, so each
ref the agent reports gets its own row. The API still returns a single
`diff_status`: the first row of the chat's statuses, ordered by
`updated_at`, newest report first. The primary ref changes only when the
agent reports a ref: refreshes, claims, backoffs and URL writes do not
advance `updated_at`, so they cannot reorder the primary.

The gitsync worker now claims, backs off and clears rows by the full
key, not the whole chat. When the diff GET discovers a PR URL, it
updates the existing ref row and no longer inserts one. Telemetry picks
one row per chat.

**NOTE**: Migration `000606` swaps the primary key under an ACCESS
EXCLUSIVE lock. The table is small. Existing rows already fit the new
key, so no data is rewritten. The down migration keeps only the primary
row per chat. Stop old coderd instances before migrating: their `ON
CONFLICT (chat_id)` upserts fail once the old key is gone.

Generated by Coder Agents.
This commit is contained in:
Danielle Maywood
2026-10-01 10:59:27 +01:00
committed by GitHub
parent decf1ddc3b
commit 63a95d87f3
19 changed files with 749 additions and 215 deletions
+20 -11
View File
@@ -3299,15 +3299,6 @@ func (q *querier) GetChatDesktopEnabled(ctx context.Context) (bool, error) {
return q.db.GetChatDesktopEnabled(ctx)
}
func (q *querier) GetChatDiffStatusByChatID(ctx context.Context, chatID uuid.UUID) (database.ChatDiffStatus, error) {
// Authorize read on the parent chat.
_, err := q.GetChatByID(ctx, chatID)
if err != nil {
return database.ChatDiffStatus{}, err
}
return q.db.GetChatDiffStatusByChatID(ctx, chatID)
}
func (q *querier) GetChatDiffStatusSummary(ctx context.Context) (database.GetChatDiffStatusSummaryRow, error) {
// Telemetry queries are called from system contexts only.
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
@@ -3316,6 +3307,15 @@ func (q *querier) GetChatDiffStatusSummary(ctx context.Context) (database.GetCha
return q.db.GetChatDiffStatusSummary(ctx)
}
func (q *querier) GetChatDiffStatusesByChatID(ctx context.Context, chatID uuid.UUID) ([]database.ChatDiffStatus, error) {
// Authorize read on the parent chat.
_, err := q.GetChatByID(ctx, chatID)
if err != nil {
return nil, err
}
return q.db.GetChatDiffStatusesByChatID(ctx, chatID)
}
func (q *querier) GetChatDiffStatusesByChatIDs(ctx context.Context, chatIDs []uuid.UUID) ([]database.ChatDiffStatus, error) {
if len(chatIDs) == 0 {
return []database.ChatDiffStatus{}, nil
@@ -7555,6 +7555,17 @@ func (q *querier) UpdateChatDebugStep(ctx context.Context, arg database.UpdateCh
return q.db.UpdateChatDebugStep(ctx, arg)
}
func (q *querier) UpdateChatDiffStatusReferenceURL(ctx context.Context, arg database.UpdateChatDiffStatusReferenceURLParams) error {
chat, err := q.db.GetChatByID(ctx, arg.ChatID)
if err != nil {
return err
}
if err := q.authorizeContext(ctx, policy.ActionUpdate, chat); err != nil {
return err
}
return q.db.UpdateChatDiffStatusReferenceURL(ctx, arg)
}
func (q *querier) UpdateChatExecutionState(ctx context.Context, arg database.UpdateChatExecutionStateParams) (database.Chat, error) {
chat, err := q.db.GetChatByID(ctx, arg.ID)
if err != nil {
@@ -9083,7 +9094,6 @@ func (q *querier) UpsertChatDesktopEnabled(ctx context.Context, enableDesktop bo
}
func (q *querier) UpsertChatDiffStatus(ctx context.Context, arg database.UpsertChatDiffStatusParams) (database.ChatDiffStatus, error) {
// Authorize update on the parent chat.
chat, err := q.db.GetChatByID(ctx, arg.ChatID)
if err != nil {
return database.ChatDiffStatus{}, err
@@ -9095,7 +9105,6 @@ func (q *querier) UpsertChatDiffStatus(ctx context.Context, arg database.UpsertC
}
func (q *querier) UpsertChatDiffStatusReference(ctx context.Context, arg database.UpsertChatDiffStatusReferenceParams) (database.ChatDiffStatus, error) {
// Authorize update on the parent chat.
chat, err := q.db.GetChatByID(ctx, arg.ChatID)
if err != nil {
return database.ChatDiffStatus{}, err
+16 -3
View File
@@ -1074,12 +1074,12 @@ func (s *MethodTestSuite) TestChats() {
dbm.EXPECT().SoftDeleteContextFileMessages(gomock.Any(), chat.ID).Return(nil).AnyTimes()
check.Args(chat.ID).Asserts(chat, policy.ActionUpdate).Returns()
}))
s.Run("GetChatDiffStatusByChatID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
s.Run("GetChatDiffStatusesByChatID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
chat := testutil.Fake(s.T(), faker, database.Chat{})
diffStatus := testutil.Fake(s.T(), faker, database.ChatDiffStatus{ChatID: chat.ID})
dbm.EXPECT().GetChatByID(gomock.Any(), chat.ID).Return(chat, nil).AnyTimes()
dbm.EXPECT().GetChatDiffStatusByChatID(gomock.Any(), chat.ID).Return(diffStatus, nil).AnyTimes()
check.Args(chat.ID).Asserts(chat, policy.ActionRead).Returns(diffStatus)
dbm.EXPECT().GetChatDiffStatusesByChatID(gomock.Any(), chat.ID).Return([]database.ChatDiffStatus{diffStatus}, nil).AnyTimes()
check.Args(chat.ID).Asserts(chat, policy.ActionRead).Returns([]database.ChatDiffStatus{diffStatus})
}))
s.Run("GetChatDiffStatusesByChatIDs", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
chatA := testutil.Fake(s.T(), faker, database.Chat{})
@@ -1819,6 +1819,19 @@ func (s *MethodTestSuite) TestChats() {
dbm.EXPECT().UpsertChatDiffStatus(gomock.Any(), arg).Return(diffStatus, nil).AnyTimes()
check.Args(arg).Asserts(chat, policy.ActionUpdate).Returns(diffStatus)
}))
s.Run("UpdateChatDiffStatusReferenceURL", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
chat := testutil.Fake(s.T(), faker, database.Chat{})
arg := database.UpdateChatDiffStatusReferenceURLParams{
ChatID: chat.ID,
Url: "https://example.com/pr/123",
GitBranch: "feature/test",
GitRemoteOrigin: "origin",
StaleAt: dbtime.Now().Add(time.Hour),
}
dbm.EXPECT().GetChatByID(gomock.Any(), chat.ID).Return(chat, nil).AnyTimes()
dbm.EXPECT().UpdateChatDiffStatusReferenceURL(gomock.Any(), arg).Return(nil).AnyTimes()
check.Args(arg).Asserts(chat, policy.ActionUpdate).Returns()
}))
s.Run("UpsertChatDiffStatusReference", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
chat := testutil.Fake(s.T(), faker, database.Chat{})
arg := database.UpsertChatDiffStatusReferenceParams{
+17 -8
View File
@@ -1552,14 +1552,6 @@ func (m queryMetricsStore) GetChatDesktopEnabled(ctx context.Context) (bool, err
return r0, r1
}
func (m queryMetricsStore) GetChatDiffStatusByChatID(ctx context.Context, chatID uuid.UUID) (database.ChatDiffStatus, error) {
start := time.Now()
r0, r1 := m.s.GetChatDiffStatusByChatID(ctx, chatID)
m.queryLatencies.WithLabelValues("GetChatDiffStatusByChatID").Observe(time.Since(start).Seconds())
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetChatDiffStatusByChatID").Inc()
return r0, r1
}
func (m queryMetricsStore) GetChatDiffStatusSummary(ctx context.Context) (database.GetChatDiffStatusSummaryRow, error) {
start := time.Now()
r0, r1 := m.s.GetChatDiffStatusSummary(ctx)
@@ -1568,6 +1560,14 @@ func (m queryMetricsStore) GetChatDiffStatusSummary(ctx context.Context) (databa
return r0, r1
}
func (m queryMetricsStore) GetChatDiffStatusesByChatID(ctx context.Context, chatID uuid.UUID) ([]database.ChatDiffStatus, error) {
start := time.Now()
r0, r1 := m.s.GetChatDiffStatusesByChatID(ctx, chatID)
m.queryLatencies.WithLabelValues("GetChatDiffStatusesByChatID").Observe(time.Since(start).Seconds())
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetChatDiffStatusesByChatID").Inc()
return r0, r1
}
func (m queryMetricsStore) GetChatDiffStatusesByChatIDs(ctx context.Context, chatIDs []uuid.UUID) ([]database.ChatDiffStatus, error) {
start := time.Now()
r0, r1 := m.s.GetChatDiffStatusesByChatIDs(ctx, chatIDs)
@@ -5336,6 +5336,15 @@ func (m queryMetricsStore) UpdateChatDebugStep(ctx context.Context, arg database
return r0, r1
}
func (m queryMetricsStore) UpdateChatDiffStatusReferenceURL(ctx context.Context, arg database.UpdateChatDiffStatusReferenceURLParams) error {
start := time.Now()
defer func() {
m.queryLatencies.WithLabelValues("UpdateChatDiffStatusReferenceURL").Observe(time.Since(start).Seconds())
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "UpdateChatDiffStatusReferenceURL").Inc()
}()
return m.s.UpdateChatDiffStatusReferenceURL(ctx, arg)
}
func (m queryMetricsStore) UpdateChatExecutionState(ctx context.Context, arg database.UpdateChatExecutionStateParams) (database.Chat, error) {
start := time.Now()
r0, r1 := m.s.UpdateChatExecutionState(ctx, arg)
+29 -15
View File
@@ -2908,21 +2908,6 @@ func (mr *MockStoreMockRecorder) GetChatDesktopEnabled(ctx any) *gomock.Call {
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatDesktopEnabled", reflect.TypeOf((*MockStore)(nil).GetChatDesktopEnabled), ctx)
}
// GetChatDiffStatusByChatID mocks base method.
func (m *MockStore) GetChatDiffStatusByChatID(ctx context.Context, chatID uuid.UUID) (database.ChatDiffStatus, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetChatDiffStatusByChatID", ctx, chatID)
ret0, _ := ret[0].(database.ChatDiffStatus)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetChatDiffStatusByChatID indicates an expected call of GetChatDiffStatusByChatID.
func (mr *MockStoreMockRecorder) GetChatDiffStatusByChatID(ctx, chatID any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatDiffStatusByChatID", reflect.TypeOf((*MockStore)(nil).GetChatDiffStatusByChatID), ctx, chatID)
}
// GetChatDiffStatusSummary mocks base method.
func (m *MockStore) GetChatDiffStatusSummary(ctx context.Context) (database.GetChatDiffStatusSummaryRow, error) {
m.ctrl.T.Helper()
@@ -2938,6 +2923,21 @@ func (mr *MockStoreMockRecorder) GetChatDiffStatusSummary(ctx any) *gomock.Call
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatDiffStatusSummary", reflect.TypeOf((*MockStore)(nil).GetChatDiffStatusSummary), ctx)
}
// GetChatDiffStatusesByChatID mocks base method.
func (m *MockStore) GetChatDiffStatusesByChatID(ctx context.Context, chatID uuid.UUID) ([]database.ChatDiffStatus, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetChatDiffStatusesByChatID", ctx, chatID)
ret0, _ := ret[0].([]database.ChatDiffStatus)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetChatDiffStatusesByChatID indicates an expected call of GetChatDiffStatusesByChatID.
func (mr *MockStoreMockRecorder) GetChatDiffStatusesByChatID(ctx, chatID any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatDiffStatusesByChatID", reflect.TypeOf((*MockStore)(nil).GetChatDiffStatusesByChatID), ctx, chatID)
}
// GetChatDiffStatusesByChatIDs mocks base method.
func (m *MockStore) GetChatDiffStatusesByChatIDs(ctx context.Context, chatIds []uuid.UUID) ([]database.ChatDiffStatus, error) {
m.ctrl.T.Helper()
@@ -10117,6 +10117,20 @@ func (mr *MockStoreMockRecorder) UpdateChatDebugStep(ctx, arg any) *gomock.Call
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateChatDebugStep", reflect.TypeOf((*MockStore)(nil).UpdateChatDebugStep), ctx, arg)
}
// UpdateChatDiffStatusReferenceURL mocks base method.
func (m *MockStore) UpdateChatDiffStatusReferenceURL(ctx context.Context, arg database.UpdateChatDiffStatusReferenceURLParams) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "UpdateChatDiffStatusReferenceURL", ctx, arg)
ret0, _ := ret[0].(error)
return ret0
}
// UpdateChatDiffStatusReferenceURL indicates an expected call of UpdateChatDiffStatusReferenceURL.
func (mr *MockStoreMockRecorder) UpdateChatDiffStatusReferenceURL(ctx, arg any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateChatDiffStatusReferenceURL", reflect.TypeOf((*MockStore)(nil).UpdateChatDiffStatusReferenceURL), ctx, arg)
}
// UpdateChatExecutionState mocks base method.
func (m *MockStore) UpdateChatExecutionState(ctx context.Context, arg database.UpdateChatExecutionStateParams) (database.Chat, error) {
m.ctrl.T.Helper()
+1 -1
View File
@@ -4322,7 +4322,7 @@ ALTER TABLE ONLY chat_debug_steps
ADD CONSTRAINT chat_debug_steps_pkey PRIMARY KEY (id);
ALTER TABLE ONLY chat_diff_statuses
ADD CONSTRAINT chat_diff_statuses_pkey PRIMARY KEY (chat_id);
ADD CONSTRAINT chat_diff_statuses_pkey PRIMARY KEY (chat_id, git_remote_origin, git_branch);
ALTER TABLE ONLY chat_file_links
ADD CONSTRAINT chat_file_links_chat_id_file_id_key UNIQUE (chat_id, file_id);
@@ -0,0 +1,25 @@
-- The old key allows one row per chat, so keep the primary row and
-- drop the rest. The primary is the first row of the same ordering
-- the API uses: newest report first, then origin and branch.
DELETE FROM chat_diff_statuses
WHERE (chat_id, git_remote_origin, git_branch) IN (
SELECT chat_id, git_remote_origin, git_branch
FROM (
SELECT
chat_id,
git_remote_origin,
git_branch,
ROW_NUMBER() OVER (
PARTITION BY chat_id
ORDER BY updated_at DESC, git_remote_origin, git_branch
) AS rank
FROM chat_diff_statuses
) ranked
WHERE ranked.rank > 1
);
ALTER TABLE chat_diff_statuses
DROP CONSTRAINT chat_diff_statuses_pkey;
ALTER TABLE chat_diff_statuses
ADD PRIMARY KEY (chat_id);
@@ -0,0 +1,16 @@
-- Remote URLs can carry a token, such as https://<token>@github.com/o/r.git.
-- Strip it with the same pattern as originCredentials in coderd/x/gitsync.
-- Each chat still has one row here, so this cannot create duplicate keys.
UPDATE chat_diff_statuses
SET git_remote_origin = regexp_replace(git_remote_origin, '^(https?://)[^/]*@', '\1')
WHERE git_remote_origin ~ '^https?://[^/]*@';
-- One row per chat, origin, and branch. This lets a chat track
-- more than one pull request. Both columns are NOT NULL with a
-- default of '', and the old key allowed one row per chat, so
-- existing rows already obey the new key.
ALTER TABLE chat_diff_statuses
DROP CONSTRAINT chat_diff_statuses_pkey;
ALTER TABLE chat_diff_statuses
ADD PRIMARY KEY (chat_id, git_remote_origin, git_branch);
@@ -404,6 +404,7 @@ func TestMigrationChain(t *testing.T) {
{"Migration000590WorkspaceAgentSessionCounts", 590, testMigration000590WorkspaceAgentSessionCounts},
{"Migration000595RemoveTaskPermissions", 595, testMigration000595RemoveTaskPermissions},
{"Migration000602RestoreAgentsAccessDefaultRole", 602, testMigration000602RestoreAgentsAccessDefaultRole},
{"Migration000606ChatDiffStatusOriginCredentials", 606, testMigration000606ChatDiffStatusOriginCredentials},
}
for _, step := range steps {
stepTo(step.version - 1)
@@ -1762,6 +1763,84 @@ func testMigration000602RestoreAgentsAccessDefaultRole(t *testing.T, sqlDB *sql.
assertDefaults(true)
}
func testMigration000606ChatDiffStatusOriginCredentials(t *testing.T, sqlDB *sql.DB, next migrationStepper) {
const migrationVersion = 606
ctx := testutil.Context(t, testutil.WaitLong)
ownerID, orgID, providerID, modelConfigID := uuid.New(), uuid.New(), uuid.New(), uuid.New()
for _, fixture := range []struct {
query string
args []any
}{
{
"INSERT INTO users (id, username, email, hashed_password, created_at, updated_at) VALUES ($1, $2, $3, '', NOW(), NOW())",
[]any{ownerID, ownerID.String(), ownerID.String() + "@test.com"},
},
{
"INSERT INTO organizations (id, name, display_name, description, created_at, updated_at, default_org_member_roles) VALUES ($1, $2, '', '', NOW(), NOW(), '{}')",
[]any{orgID, orgID.String()},
},
{
"INSERT INTO ai_providers (id, type, name, base_url) VALUES ($1, 'openai', $2, 'https://example.com')",
[]any{providerID, providerID.String()},
},
{
"INSERT INTO chat_model_configs (id, model, context_limit, compression_threshold, ai_provider_id, organization_id) VALUES ($1, 'model', 1000, 50, $2, $3)",
[]any{modelConfigID, providerID, orgID},
},
} {
_, err := sqlDB.ExecContext(ctx, fixture.query, fixture.args...)
require.NoError(t, err)
}
tests := []struct {
name string
origin string
want string
}{
{"strips a token", "https://ghp_abc@github.com/o/r.git", "https://github.com/o/r.git"},
{"strips a user and password", "https://oauth2:tok@gitlab.com/g/r.git", "https://gitlab.com/g/r.git"},
{"strips a password with an at sign", "https://user:p@ss@github.com/o/r.git", "https://github.com/o/r.git"},
{"strips over http", "http://tok@git.example.com/o/r.git", "http://git.example.com/o/r.git"},
{"strips and keeps the port", "https://tok@github.com:443/o/r", "https://github.com:443/o/r"},
{"strips without a path", "https://tok@github.com", "https://github.com"},
{"strips and keeps an at sign in the path", "https://tok@github.com/o/r@v1", "https://github.com/o/r@v1"},
{"keeps an origin without credentials", "https://github.com/o/r.git", "https://github.com/o/r.git"},
{"keeps an at sign in the path", "https://github.com/o/r@v1", "https://github.com/o/r@v1"},
{"keeps an scp-style SSH user", "git@github.com:o/r.git", "git@github.com:o/r.git"},
{"keeps an SSH URL user", "ssh://git@github.com/o/r.git", "ssh://git@github.com/o/r.git"},
{"keeps an empty origin", "", ""},
}
// The old key is chat_id, so each case needs its own chat.
chatIDs := make([]uuid.UUID, len(tests))
for i, tt := range tests {
chatIDs[i] = uuid.New()
_, err := sqlDB.ExecContext(ctx,
"INSERT INTO chats (id, owner_id, organization_id, last_model_config_id) VALUES ($1, $2, $3, $4)",
chatIDs[i], ownerID, orgID, modelConfigID)
require.NoError(t, err)
_, err = sqlDB.ExecContext(ctx,
"INSERT INTO chat_diff_statuses (chat_id, git_remote_origin) VALUES ($1, $2)",
chatIDs[i], tt.origin)
require.NoError(t, err)
}
version, _, err := next()
require.NoError(t, err)
require.EqualValues(t, migrationVersion, version)
for i, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var got string
err := sqlDB.QueryRowContext(ctx, "SELECT git_remote_origin FROM chat_diff_statuses WHERE chat_id = $1", chatIDs[i]).Scan(&got)
require.NoError(t, err)
require.Equal(t, tt.want, got)
})
}
}
func TestMigration000504AIProvidersBackfill(t *testing.T) {
t.Parallel()
+2 -1
View File
@@ -459,7 +459,6 @@ type sqlcQuerier interface {
GetChatDebugRunsByChatID(ctx context.Context, arg GetChatDebugRunsByChatIDParams) ([]ChatDebugRun, error)
GetChatDebugStepsByRunID(ctx context.Context, runID uuid.UUID) ([]ChatDebugStep, error)
GetChatDesktopEnabled(ctx context.Context) (bool, error)
GetChatDiffStatusByChatID(ctx context.Context, chatID uuid.UUID) (ChatDiffStatus, error)
// Returns aggregate PR counts across all agent chats for telemetry.
// Deduplicates by PR URL so forked chats referencing the same pull
// request are counted once (using the most recently refreshed state).
@@ -467,6 +466,7 @@ type sqlcQuerier interface {
// always equals open + merged + closed; other non-NULL states are
// intentionally excluded from these aggregates.
GetChatDiffStatusSummary(ctx context.Context) (GetChatDiffStatusSummaryRow, error)
GetChatDiffStatusesByChatID(ctx context.Context, chatID uuid.UUID) ([]ChatDiffStatus, error)
GetChatDiffStatusesByChatIDs(ctx context.Context, chatIds []uuid.UUID) ([]ChatDiffStatus, error)
// Returns the chat IDs of every chat in a family (root + all children)
// in deterministic order. The id parameter must be the root id; the
@@ -1512,6 +1512,7 @@ type sqlcQuerier interface {
// parameter keeps updated_at under the caller's clock, matching
// the injectable quartz.Clock used by FinalizeStale sweeps.
UpdateChatDebugStep(ctx context.Context, arg UpdateChatDebugStepParams) (ChatDebugStep, error)
UpdateChatDiffStatusReferenceURL(ctx context.Context, arg UpdateChatDiffStatusReferenceURLParams) error
// Atomically updates the execution-state-managed fields on a chat:
// status, archived, last_error, ownership identifiers, the
// requires-action deadline, and the manual compaction request marker.
+4
View File
@@ -17466,6 +17466,8 @@ func TestGetChatsFilter(t *testing.T) {
now := time.Now()
_, err := store.UpsertChatDiffStatus(ctx, database.UpsertChatDiffStatusParams{
ChatID: chatID,
GitRemoteOrigin: "https://github.com/coder/coder.git",
GitBranch: "main",
Url: sql.NullString{String: url, Valid: true},
PullRequestState: sql.NullString{String: state, Valid: true},
PullRequestTitle: "PR " + state,
@@ -17496,6 +17498,8 @@ func TestGetChatsFilter(t *testing.T) {
// Then set PR metadata via the status upsert.
_, err := store.UpsertChatDiffStatus(ctx, database.UpsertChatDiffStatusParams{
ChatID: chatID,
GitRemoteOrigin: gitRemoteOrigin,
GitBranch: "main",
Url: sql.NullString{String: url, Valid: url != ""},
PullRequestState: sql.NullString{String: state, Valid: state != ""},
PullRequestTitle: prTitle,
+148 -81
View File
@@ -7529,15 +7529,13 @@ WITH acquired AS (
-- Claim for 5 minutes. The worker sets the real stale_at
-- after refresh. If the worker crashes, rows become eligible
-- again after this interval.
-- NOTE: updated_at is intentionally NOT touched here so
-- the worker can read it as "when was this row last
-- externally changed" (by MarkStale or a successful
-- refresh).
stale_at = NOW() + INTERVAL '5 minutes'
WHERE
chat_id IN (
(chat_id, git_remote_origin, git_branch) IN (
SELECT
cds.chat_id
cds.chat_id,
cds.git_remote_origin,
cds.git_branch
FROM
chat_diff_statuses cds
INNER JOIN
@@ -7994,22 +7992,27 @@ const backoffChatDiffStatus = `-- name: BackoffChatDiffStatus :exec
UPDATE
chat_diff_statuses
SET
-- NOTE: updated_at is intentionally NOT touched here so
-- the worker can read it as "when was this row last
-- externally changed" (by MarkStale or a successful
-- refresh).
stale_at = $1::timestamptz
WHERE
chat_id = $2::uuid
AND git_remote_origin = $3::text
AND git_branch = $4::text
`
type BackoffChatDiffStatusParams struct {
StaleAt time.Time `db:"stale_at" json:"stale_at"`
ChatID uuid.UUID `db:"chat_id" json:"chat_id"`
StaleAt time.Time `db:"stale_at" json:"stale_at"`
ChatID uuid.UUID `db:"chat_id" json:"chat_id"`
GitRemoteOrigin string `db:"git_remote_origin" json:"git_remote_origin"`
GitBranch string `db:"git_branch" json:"git_branch"`
}
func (q *sqlQuerier) BackoffChatDiffStatus(ctx context.Context, arg BackoffChatDiffStatusParams) error {
_, err := q.db.ExecContext(ctx, backoffChatDiffStatus, arg.StaleAt, arg.ChatID)
_, err := q.db.ExecContext(ctx, backoffChatDiffStatus,
arg.StaleAt,
arg.ChatID,
arg.GitRemoteOrigin,
arg.GitBranch,
)
return err
}
@@ -8077,15 +8080,24 @@ SET
stale_at = $1::timestamptz
WHERE
chat_id = $2::uuid
AND git_remote_origin = $3::text
AND git_branch = $4::text
`
type ClearChatDiffStatusPRParams struct {
StaleAt time.Time `db:"stale_at" json:"stale_at"`
ChatID uuid.UUID `db:"chat_id" json:"chat_id"`
StaleAt time.Time `db:"stale_at" json:"stale_at"`
ChatID uuid.UUID `db:"chat_id" json:"chat_id"`
GitRemoteOrigin string `db:"git_remote_origin" json:"git_remote_origin"`
GitBranch string `db:"git_branch" json:"git_branch"`
}
func (q *sqlQuerier) ClearChatDiffStatusPR(ctx context.Context, arg ClearChatDiffStatusPRParams) error {
_, err := q.db.ExecContext(ctx, clearChatDiffStatusPR, arg.StaleAt, arg.ChatID)
_, err := q.db.ExecContext(ctx, clearChatDiffStatusPR,
arg.StaleAt,
arg.ChatID,
arg.GitRemoteOrigin,
arg.GitBranch,
)
return err
}
@@ -8856,46 +8868,6 @@ func (q *sqlQuerier) GetChatByIDForUpdate(ctx context.Context, id uuid.UUID) (Ch
return i, err
}
const getChatDiffStatusByChatID = `-- name: GetChatDiffStatusByChatID :one
SELECT
chat_id, url, pull_request_state, changes_requested, additions, deletions, changed_files, refreshed_at, stale_at, created_at, updated_at, git_branch, git_remote_origin, pull_request_title, pull_request_draft, author_login, author_avatar_url, base_branch, pr_number, commits, approved, reviewer_count, head_branch
FROM
chat_diff_statuses
WHERE
chat_id = $1::uuid
`
func (q *sqlQuerier) GetChatDiffStatusByChatID(ctx context.Context, chatID uuid.UUID) (ChatDiffStatus, error) {
row := q.db.QueryRowContext(ctx, getChatDiffStatusByChatID, chatID)
var i ChatDiffStatus
err := row.Scan(
&i.ChatID,
&i.Url,
&i.PullRequestState,
&i.ChangesRequested,
&i.Additions,
&i.Deletions,
&i.ChangedFiles,
&i.RefreshedAt,
&i.StaleAt,
&i.CreatedAt,
&i.UpdatedAt,
&i.GitBranch,
&i.GitRemoteOrigin,
&i.PullRequestTitle,
&i.PullRequestDraft,
&i.AuthorLogin,
&i.AuthorAvatarUrl,
&i.BaseBranch,
&i.PrNumber,
&i.Commits,
&i.Approved,
&i.ReviewerCount,
&i.HeadBranch,
)
return i, err
}
const getChatDiffStatusSummary = `-- name: GetChatDiffStatusSummary :one
WITH deduped AS (
SELECT DISTINCT ON (COALESCE(NULLIF(cds.url, ''), c.id::text))
@@ -8903,7 +8875,7 @@ WITH deduped AS (
FROM chat_diff_statuses cds
JOIN chats c ON c.id = cds.chat_id
WHERE cds.pull_request_state IN ('open', 'merged', 'closed')
ORDER BY COALESCE(NULLIF(cds.url, ''), c.id::text), cds.updated_at DESC, c.id DESC
ORDER BY COALESCE(NULLIF(cds.url, ''), c.id::text), cds.refreshed_at DESC NULLS LAST, cds.updated_at DESC, c.id DESC
)
SELECT
COUNT(*)::bigint AS total,
@@ -8938,6 +8910,66 @@ func (q *sqlQuerier) GetChatDiffStatusSummary(ctx context.Context) (GetChatDiffS
return i, err
}
const getChatDiffStatusesByChatID = `-- name: GetChatDiffStatusesByChatID :many
SELECT
chat_id, url, pull_request_state, changes_requested, additions, deletions, changed_files, refreshed_at, stale_at, created_at, updated_at, git_branch, git_remote_origin, pull_request_title, pull_request_draft, author_login, author_avatar_url, base_branch, pr_number, commits, approved, reviewer_count, head_branch
FROM
chat_diff_statuses
WHERE
chat_id = $1::uuid
ORDER BY
updated_at DESC,
git_remote_origin,
git_branch
`
func (q *sqlQuerier) GetChatDiffStatusesByChatID(ctx context.Context, chatID uuid.UUID) ([]ChatDiffStatus, error) {
rows, err := q.db.QueryContext(ctx, getChatDiffStatusesByChatID, chatID)
if err != nil {
return nil, err
}
defer rows.Close()
var items []ChatDiffStatus
for rows.Next() {
var i ChatDiffStatus
if err := rows.Scan(
&i.ChatID,
&i.Url,
&i.PullRequestState,
&i.ChangesRequested,
&i.Additions,
&i.Deletions,
&i.ChangedFiles,
&i.RefreshedAt,
&i.StaleAt,
&i.CreatedAt,
&i.UpdatedAt,
&i.GitBranch,
&i.GitRemoteOrigin,
&i.PullRequestTitle,
&i.PullRequestDraft,
&i.AuthorLogin,
&i.AuthorAvatarUrl,
&i.BaseBranch,
&i.PrNumber,
&i.Commits,
&i.Approved,
&i.ReviewerCount,
&i.HeadBranch,
); err != nil {
return nil, err
}
items = append(items, i)
}
if err := rows.Close(); err != nil {
return nil, err
}
if err := rows.Err(); err != nil {
return nil, err
}
return items, nil
}
const getChatDiffStatusesByChatIDs = `-- name: GetChatDiffStatusesByChatIDs :many
SELECT
chat_id, url, pull_request_state, changes_requested, additions, deletions, changed_files, refreshed_at, stale_at, created_at, updated_at, git_branch, git_remote_origin, pull_request_title, pull_request_draft, author_login, author_avatar_url, base_branch, pr_number, commits, approved, reviewer_count, head_branch
@@ -8945,6 +8977,10 @@ FROM
chat_diff_statuses
WHERE
chat_id = ANY($1::uuid[])
ORDER BY
updated_at DESC,
git_remote_origin,
git_branch
`
func (q *sqlQuerier) GetChatDiffStatusesByChatIDs(ctx context.Context, chatIds []uuid.UUID) ([]ChatDiffStatus, error) {
@@ -10622,7 +10658,7 @@ func (q *sqlQuerier) GetChatsByWorkspaceIDs(ctx context.Context, ids []uuid.UUID
}
const getChatsUpdatedAfter = `-- name: GetChatsUpdatedAfter :many
SELECT
SELECT DISTINCT ON (c.id)
c.id, c.owner_id, c.organization_id, c.created_at, c.updated_at, c.status,
(c.parent_chat_id IS NOT NULL)::bool AS has_parent,
c.root_chat_id, c.workspace_id,
@@ -10631,6 +10667,7 @@ SELECT
FROM chats c
LEFT JOIN chat_diff_statuses cds ON cds.chat_id = c.id
WHERE c.updated_at > $1
ORDER BY c.id, cds.updated_at DESC NULLS LAST, cds.git_remote_origin, cds.git_branch
`
type GetChatsUpdatedAfterRow struct {
@@ -12749,6 +12786,37 @@ func (q *sqlQuerier) UpdateChatByID(ctx context.Context, arg UpdateChatByIDParam
return i, err
}
const updateChatDiffStatusReferenceURL = `-- name: UpdateChatDiffStatusReferenceURL :exec
UPDATE
chat_diff_statuses
SET
url = $1::text,
stale_at = $2::timestamptz
WHERE
chat_id = $3::uuid
AND git_remote_origin = $4::text
AND git_branch = $5::text
`
type UpdateChatDiffStatusReferenceURLParams struct {
Url string `db:"url" json:"url"`
StaleAt time.Time `db:"stale_at" json:"stale_at"`
ChatID uuid.UUID `db:"chat_id" json:"chat_id"`
GitRemoteOrigin string `db:"git_remote_origin" json:"git_remote_origin"`
GitBranch string `db:"git_branch" json:"git_branch"`
}
func (q *sqlQuerier) UpdateChatDiffStatusReferenceURL(ctx context.Context, arg UpdateChatDiffStatusReferenceURLParams) error {
_, err := q.db.ExecContext(ctx, updateChatDiffStatusReferenceURL,
arg.Url,
arg.StaleAt,
arg.ChatID,
arg.GitRemoteOrigin,
arg.GitBranch,
)
return err
}
const updateChatExecutionState = `-- name: UpdateChatExecutionState :one
WITH updated_chat AS (
UPDATE chats
@@ -14183,6 +14251,8 @@ const upsertChatDiffStatus = `-- name: UpsertChatDiffStatus :one
INSERT INTO chat_diff_statuses (
chat_id,
url,
git_branch,
git_remote_origin,
pull_request_state,
pull_request_title,
pull_request_draft,
@@ -14205,23 +14275,25 @@ INSERT INTO chat_diff_statuses (
$2::text,
$3::text,
$4::text,
$5::boolean,
$6::boolean,
$7::integer,
$8::integer,
$5::text,
$6::text,
$7::boolean,
$8::boolean,
$9::integer,
$10::text,
$11::text,
$10::integer,
$11::integer,
$12::text,
$13::text,
$14::integer,
$15::integer,
$16::boolean,
$14::text,
$15::text,
$16::integer,
$17::integer,
$18::timestamptz,
$19::timestamptz
$18::boolean,
$19::integer,
$20::timestamptz,
$21::timestamptz
)
ON CONFLICT (chat_id) DO UPDATE
ON CONFLICT (chat_id, git_remote_origin, git_branch) DO UPDATE
SET
url = EXCLUDED.url,
pull_request_state = EXCLUDED.pull_request_state,
@@ -14240,8 +14312,7 @@ SET
approved = EXCLUDED.approved,
reviewer_count = EXCLUDED.reviewer_count,
refreshed_at = EXCLUDED.refreshed_at,
stale_at = EXCLUDED.stale_at,
updated_at = NOW()
stale_at = EXCLUDED.stale_at
RETURNING
chat_id, url, pull_request_state, changes_requested, additions, deletions, changed_files, refreshed_at, stale_at, created_at, updated_at, git_branch, git_remote_origin, pull_request_title, pull_request_draft, author_login, author_avatar_url, base_branch, pr_number, commits, approved, reviewer_count, head_branch
`
@@ -14249,6 +14320,8 @@ RETURNING
type UpsertChatDiffStatusParams struct {
ChatID uuid.UUID `db:"chat_id" json:"chat_id"`
Url sql.NullString `db:"url" json:"url"`
GitBranch string `db:"git_branch" json:"git_branch"`
GitRemoteOrigin string `db:"git_remote_origin" json:"git_remote_origin"`
PullRequestState sql.NullString `db:"pull_request_state" json:"pull_request_state"`
PullRequestTitle string `db:"pull_request_title" json:"pull_request_title"`
PullRequestDraft bool `db:"pull_request_draft" json:"pull_request_draft"`
@@ -14272,6 +14345,8 @@ func (q *sqlQuerier) UpsertChatDiffStatus(ctx context.Context, arg UpsertChatDif
row := q.db.QueryRowContext(ctx, upsertChatDiffStatus,
arg.ChatID,
arg.Url,
arg.GitBranch,
arg.GitRemoteOrigin,
arg.PullRequestState,
arg.PullRequestTitle,
arg.PullRequestDraft,
@@ -14333,20 +14408,12 @@ INSERT INTO chat_diff_statuses (
$4::text,
$5::timestamptz
)
ON CONFLICT (chat_id) DO UPDATE
ON CONFLICT (chat_id, git_remote_origin, git_branch) DO UPDATE
SET
url = CASE
WHEN EXCLUDED.url IS NOT NULL THEN EXCLUDED.url
ELSE chat_diff_statuses.url
END,
git_branch = CASE
WHEN EXCLUDED.git_branch != '' THEN EXCLUDED.git_branch
ELSE chat_diff_statuses.git_branch
END,
git_remote_origin = CASE
WHEN EXCLUDED.git_remote_origin != '' THEN EXCLUDED.git_remote_origin
ELSE chat_diff_statuses.git_remote_origin
END,
stale_at = EXCLUDED.stale_at,
updated_at = NOW()
RETURNING
+43 -30
View File
@@ -1940,13 +1940,17 @@ WHERE
AND status = 'running'::chat_status
RETURNING id;
-- name: GetChatDiffStatusByChatID :one
-- name: GetChatDiffStatusesByChatID :many
SELECT
*
FROM
chat_diff_statuses
WHERE
chat_id = @chat_id::uuid;
chat_id = @chat_id::uuid
ORDER BY
updated_at DESC,
git_remote_origin,
git_branch;
-- name: GetChatDiffStatusesByChatIDs :many
SELECT
@@ -1954,7 +1958,22 @@ SELECT
FROM
chat_diff_statuses
WHERE
chat_id = ANY(@chat_ids::uuid[]);
chat_id = ANY(@chat_ids::uuid[])
ORDER BY
updated_at DESC,
git_remote_origin,
git_branch;
-- name: UpdateChatDiffStatusReferenceURL :exec
UPDATE
chat_diff_statuses
SET
url = @url::text,
stale_at = @stale_at::timestamptz
WHERE
chat_id = @chat_id::uuid
AND git_remote_origin = @git_remote_origin::text
AND git_branch = @git_branch::text;
-- name: UpsertChatDiffStatusReference :one
INSERT INTO chat_diff_statuses (
@@ -1970,20 +1989,12 @@ INSERT INTO chat_diff_statuses (
@git_remote_origin::text,
@stale_at::timestamptz
)
ON CONFLICT (chat_id) DO UPDATE
ON CONFLICT (chat_id, git_remote_origin, git_branch) DO UPDATE
SET
url = CASE
WHEN EXCLUDED.url IS NOT NULL THEN EXCLUDED.url
ELSE chat_diff_statuses.url
END,
git_branch = CASE
WHEN EXCLUDED.git_branch != '' THEN EXCLUDED.git_branch
ELSE chat_diff_statuses.git_branch
END,
git_remote_origin = CASE
WHEN EXCLUDED.git_remote_origin != '' THEN EXCLUDED.git_remote_origin
ELSE chat_diff_statuses.git_remote_origin
END,
stale_at = EXCLUDED.stale_at,
updated_at = NOW()
RETURNING
@@ -1993,6 +2004,8 @@ RETURNING
INSERT INTO chat_diff_statuses (
chat_id,
url,
git_branch,
git_remote_origin,
pull_request_state,
pull_request_title,
pull_request_draft,
@@ -2013,6 +2026,8 @@ INSERT INTO chat_diff_statuses (
) VALUES (
@chat_id::uuid,
sqlc.narg('url')::text,
@git_branch::text,
@git_remote_origin::text,
sqlc.narg('pull_request_state')::text,
@pull_request_title::text,
@pull_request_draft::boolean,
@@ -2031,7 +2046,7 @@ INSERT INTO chat_diff_statuses (
@refreshed_at::timestamptz,
@stale_at::timestamptz
)
ON CONFLICT (chat_id) DO UPDATE
ON CONFLICT (chat_id, git_remote_origin, git_branch) DO UPDATE
SET
url = EXCLUDED.url,
pull_request_state = EXCLUDED.pull_request_state,
@@ -2050,8 +2065,7 @@ SET
approved = EXCLUDED.approved,
reviewer_count = EXCLUDED.reviewer_count,
refreshed_at = EXCLUDED.refreshed_at,
stale_at = EXCLUDED.stale_at,
updated_at = NOW()
stale_at = EXCLUDED.stale_at
RETURNING
*;
@@ -2275,15 +2289,13 @@ WITH acquired AS (
-- Claim for 5 minutes. The worker sets the real stale_at
-- after refresh. If the worker crashes, rows become eligible
-- again after this interval.
-- NOTE: updated_at is intentionally NOT touched here so
-- the worker can read it as "when was this row last
-- externally changed" (by MarkStale or a successful
-- refresh).
stale_at = NOW() + INTERVAL '5 minutes'
WHERE
chat_id IN (
(chat_id, git_remote_origin, git_branch) IN (
SELECT
cds.chat_id
cds.chat_id,
cds.git_remote_origin,
cds.git_branch
FROM
chat_diff_statuses cds
INNER JOIN
@@ -2314,13 +2326,11 @@ INNER JOIN
UPDATE
chat_diff_statuses
SET
-- NOTE: updated_at is intentionally NOT touched here so
-- the worker can read it as "when was this row last
-- externally changed" (by MarkStale or a successful
-- refresh).
stale_at = @stale_at::timestamptz
WHERE
chat_id = @chat_id::uuid;
chat_id = @chat_id::uuid
AND git_remote_origin = @git_remote_origin::text
AND git_branch = @git_branch::text;
-- name: ClearChatDiffStatusPR :exec
UPDATE
@@ -2344,7 +2354,9 @@ SET
reviewer_count = NULL,
stale_at = @stale_at::timestamptz
WHERE
chat_id = @chat_id::uuid;
chat_id = @chat_id::uuid
AND git_remote_origin = @git_remote_origin::text
AND git_branch = @git_branch::text;
-- name: GetChatDiffStatusSummary :one
-- Returns aggregate PR counts across all agent chats for telemetry.
@@ -2359,7 +2371,7 @@ WITH deduped AS (
FROM chat_diff_statuses cds
JOIN chats c ON c.id = cds.chat_id
WHERE cds.pull_request_state IN ('open', 'merged', 'closed')
ORDER BY COALESCE(NULLIF(cds.url, ''), c.id::text), cds.updated_at DESC, c.id DESC
ORDER BY COALESCE(NULLIF(cds.url, ''), c.id::text), cds.refreshed_at DESC NULLS LAST, cds.updated_at DESC, c.id DESC
)
SELECT
COUNT(*)::bigint AS total,
@@ -2414,7 +2426,7 @@ WHERE chats.id = deletable.id
-- Retrieves chats updated after the given timestamp for telemetry
-- snapshot collection. Uses updated_at so that long-running chats
-- still appear in each snapshot window while they are active.
SELECT
SELECT DISTINCT ON (c.id)
c.id, c.owner_id, c.organization_id, c.created_at, c.updated_at, c.status,
(c.parent_chat_id IS NOT NULL)::bool AS has_parent,
c.root_chat_id, c.workspace_id,
@@ -2422,7 +2434,8 @@ SELECT
cds.pull_request_state
FROM chats c
LEFT JOIN chat_diff_statuses cds ON cds.chat_id = c.id
WHERE c.updated_at > @updated_after;
WHERE c.updated_at > @updated_after
ORDER BY c.id, cds.updated_at DESC NULLS LAST, cds.git_remote_origin, cds.git_branch;
-- name: GetChatMessageSummariesPerChat :many
-- Aggregates message-level metrics per chat for messages created
+1 -1
View File
@@ -25,7 +25,7 @@ const (
UniqueChatContextResourcesPkey UniqueConstraint = "chat_context_resources_pkey" // ALTER TABLE ONLY chat_context_resources ADD CONSTRAINT chat_context_resources_pkey PRIMARY KEY (chat_id, source);
UniqueChatDebugRunsPkey UniqueConstraint = "chat_debug_runs_pkey" // ALTER TABLE ONLY chat_debug_runs ADD CONSTRAINT chat_debug_runs_pkey PRIMARY KEY (id);
UniqueChatDebugStepsPkey UniqueConstraint = "chat_debug_steps_pkey" // ALTER TABLE ONLY chat_debug_steps ADD CONSTRAINT chat_debug_steps_pkey PRIMARY KEY (id);
UniqueChatDiffStatusesPkey UniqueConstraint = "chat_diff_statuses_pkey" // ALTER TABLE ONLY chat_diff_statuses ADD CONSTRAINT chat_diff_statuses_pkey PRIMARY KEY (chat_id);
UniqueChatDiffStatusesPkey UniqueConstraint = "chat_diff_statuses_pkey" // ALTER TABLE ONLY chat_diff_statuses ADD CONSTRAINT chat_diff_statuses_pkey PRIMARY KEY (chat_id, git_remote_origin, git_branch);
UniqueChatFileLinksChatIDFileIDKey UniqueConstraint = "chat_file_links_chat_id_file_id_key" // ALTER TABLE ONLY chat_file_links ADD CONSTRAINT chat_file_links_chat_id_file_id_key UNIQUE (chat_id, file_id);
UniqueChatFileLinksFileIDKey UniqueConstraint = "chat_file_links_file_id_key" // ALTER TABLE ONLY chat_file_links ADD CONSTRAINT chat_file_links_file_id_key UNIQUE (file_id);
UniqueChatFilesPkey UniqueConstraint = "chat_files_pkey" // ALTER TABLE ONLY chat_files ADD CONSTRAINT chat_files_pkey PRIMARY KEY (id);
+30 -43
View File
@@ -663,6 +663,9 @@ func (api *API) getChatDiffStatusesByChatID(
statusesByChatID := make(map[uuid.UUID]database.ChatDiffStatus, len(statuses))
for _, status := range statuses {
if _, ok := statusesByChatID[status.ChatID]; ok {
continue
}
statusesByChatID[status.ChatID] = status
}
return statusesByChatID, nil
@@ -1579,16 +1582,16 @@ func (api *API) getChat(rw http.ResponseWriter, r *http.Request) {
// blocks the response for 200-800ms. The background gitsync
// worker keeps the cached status fresh.
var diffStatus *database.ChatDiffStatus
status, err := api.Database.GetChatDiffStatusByChatID(ctx, chat.ID)
switch {
case err == nil:
diffStatus = &status
case !xerrors.Is(err, sql.ErrNoRows):
diffStatuses, err := api.Database.GetChatDiffStatusesByChatID(ctx, chat.ID)
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
api.Logger.Error(ctx, "failed to get cached chat diff status",
slog.F("chat_id", chat.ID),
slog.Error(err),
)
}
if len(diffStatuses) > 0 {
diffStatus = &diffStatuses[0]
}
// Hydrate file metadata for all files linked to this chat.
chatFiles := api.fetchChatFileMetadata(ctx, chat.ID)
@@ -4071,10 +4074,21 @@ func (api *API) resolveChatDiffContents(
if reference.PullRequestURL != "" {
pullRequestURL := strings.TrimSpace(reference.PullRequestURL)
result.PullRequestURL = &pullRequestURL
if !found || !strings.EqualFold(strings.TrimSpace(status.Url.String), pullRequestURL) {
_, err := api.upsertChatDiffStatusReference(ctx, chat.ID, pullRequestURL, time.Now().UTC().Add(-time.Second))
// The agent's report creates the row. Discovery only fills in
// the URL, so skip until the ref was reported.
if found && (!strings.EqualFold(strings.TrimSpace(status.Url.String), pullRequestURL)) {
err := api.Database.UpdateChatDiffStatusReferenceURL(
ctx,
database.UpdateChatDiffStatusReferenceURLParams{
ChatID: status.ChatID,
GitBranch: status.GitBranch,
GitRemoteOrigin: status.GitRemoteOrigin,
StaleAt: time.Now().UTC().Add(-time.Second),
Url: pullRequestURL,
},
)
if err != nil {
return result, err
return result, xerrors.Errorf("update chat diff status reference url: %w", err)
}
}
}
@@ -4231,48 +4245,21 @@ func (api *API) buildChatRepositoryRefFromStatus(ctx context.Context, status dat
return repoRef
}
func (api *API) upsertChatDiffStatusReference(
ctx context.Context,
chatID uuid.UUID,
pullRequestURL string,
staleAt time.Time,
) (database.ChatDiffStatus, error) {
status, err := api.Database.UpsertChatDiffStatusReference(
ctx,
database.UpsertChatDiffStatusReferenceParams{
ChatID: chatID,
Url: sql.NullString{
String: pullRequestURL,
Valid: strings.TrimSpace(pullRequestURL) != "",
},
// Empty strings preserve existing values via the
// CASE expression in the SQL query.
GitBranch: "",
GitRemoteOrigin: "",
StaleAt: staleAt,
},
)
if err != nil {
return database.ChatDiffStatus{}, xerrors.Errorf("upsert chat diff status reference: %w", err)
}
return status, nil
}
func (api *API) getCachedChatDiffStatus(
ctx context.Context,
chatID uuid.UUID,
) (database.ChatDiffStatus, bool, error) {
status, err := api.Database.GetChatDiffStatusByChatID(ctx, chatID)
if err == nil {
return status, true, nil
statuses, err := api.Database.GetChatDiffStatusesByChatID(ctx, chatID)
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
return database.ChatDiffStatus{}, false, xerrors.Errorf(
"get chat diff status: %w",
err,
)
}
if xerrors.Is(err, sql.ErrNoRows) {
if len(statuses) == 0 {
return database.ChatDiffStatus{}, false, nil
}
return database.ChatDiffStatus{}, false, xerrors.Errorf(
"get chat diff status: %w",
err,
)
return statuses[0], true, nil
}
// resolveExternalAuth finds the external auth config matching the
+166 -2
View File
@@ -11,6 +11,7 @@ import (
"io"
"mime"
"net/http"
"net/http/httptest"
"net/url"
"regexp"
"slices"
@@ -28,6 +29,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/sync/errgroup"
"golang.org/x/sync/singleflight"
"golang.org/x/xerrors"
"cdr.dev/slog/v3/sloggers/slogtest"
@@ -3571,6 +3573,8 @@ func TestWatchChats(t *testing.T) {
database.UpsertChatDiffStatusParams{
ChatID: chat.ID,
Url: sql.NullString{String: "https://github.com/coder/coder/pull/99", Valid: true},
GitBranch: "feature/test",
GitRemoteOrigin: "git@github.com:coder/coder.git",
PullRequestState: sql.NullString{String: "open", Valid: true},
Additions: 42,
Deletions: 7,
@@ -11246,8 +11250,10 @@ func TestGetChatDiffStatus(t *testing.T) {
_, err = db.UpsertChatDiffStatus(
dbauthz.AsSystemRestricted(ctx),
database.UpsertChatDiffStatusParams{
ChatID: cachedStatusChat.ID,
Url: sql.NullString{},
ChatID: cachedStatusChat.ID,
GitRemoteOrigin: "git@github.com:coder/coder.git",
GitBranch: "feature/diff-status",
Url: sql.NullString{},
PullRequestState: sql.NullString{
String: " open ",
Valid: true,
@@ -11391,6 +11397,164 @@ func TestGetChatDiffContents(t *testing.T) {
require.Empty(t, diffContents.Diff)
})
t.Run("DiscoveryWriteKeysStoredRef", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
const gitToken = "test-git-token"
var githubServer *httptest.Server
githubServer = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// The provider accepts only https URLs, so the fake server
// uses TLS and every URL string uses the https scheme.
host := strings.TrimPrefix(githubServer.URL, "https://")
w.Header().Set("Content-Type", "application/json")
switch {
case strings.HasSuffix(r.URL.Path, "/pulls"):
_, _ = w.Write([]byte(`[{"html_url": "https://` + host + `/acme/project/pull/7", "number": 7}]`))
case strings.HasSuffix(r.URL.Path, "/reviews"):
_, _ = w.Write([]byte(`[]`))
case strings.Contains(r.URL.Path, "/compare/"):
w.Header().Set("Content-Type", "text/plain")
_, _ = w.Write([]byte("diff --git a/main.go b/main.go"))
default:
_, _ = w.Write([]byte(`{"default_branch": "main", "state": "open", "number": 7}`))
}
}))
t.Cleanup(githubServer.Close)
// The provider parses the origin against the fake host, so the
// stored origin must use it too.
host := strings.TrimPrefix(githubServer.URL, "https://")
origin := "https://" + host + "/acme/project.git"
prURL := "https://" + host + "/acme/project/pull/7"
rawClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
DeploymentValues: coderdtest.DeploymentValues(t),
ExternalAuthConfigs: []*externalauth.Config{
{
ID: "github-test",
Type: "github",
Regex: regexp.MustCompile(`^https?://` + regexp.QuoteMeta(host) + `(/.*)?$`),
APIBaseURL: githubServer.URL + "/api/v3",
HTTPClient: githubServer.Client(),
RefreshGroup: new(singleflight.Group),
},
},
})
aibridgedtest.StartTestAIBridgeDaemon(t.Context(), t, api, nil)
client := codersdk.NewExperimentalClient(rawClient)
db := api.Database
user := coderdtest.CreateFirstUser(t, client.Client)
modelConfig := createChatModel(t, client)
_, err := db.InsertExternalAuthLink(dbauthz.AsSystemRestricted(ctx), database.InsertExternalAuthLinkParams{
ProviderID: "github-test",
UserID: user.UserID,
OAuthAccessToken: gitToken,
OAuthExpiry: dbtime.Now().Add(24 * time.Hour),
CreatedAt: dbtime.Now(),
UpdatedAt: dbtime.Now(),
})
require.NoError(t, err)
chat := dbgen.Chat(t, db, database.Chat{
OrganizationID: user.OrganizationID,
OwnerID: user.UserID,
LastModelConfigID: modelConfig.ID,
Title: "discovery write keys stored ref",
})
// A reported ref with no stored PR URL, so the diff GET must
// resolve the PR upstream.
_, err = db.UpsertChatDiffStatusReference(
dbauthz.AsSystemRestricted(ctx),
database.UpsertChatDiffStatusReferenceParams{
ChatID: chat.ID,
Url: sql.NullString{},
GitBranch: "feature/keyed-discovery",
GitRemoteOrigin: origin,
StaleAt: time.Now().UTC().Add(time.Hour),
},
)
require.NoError(t, err)
diffContents, err := client.GetChatDiffContents(ctx, chat.ID)
require.NoError(t, err)
require.NotNil(t, diffContents.Provider)
require.NotNil(t, diffContents.PullRequestURL)
require.Equal(t, prURL, *diffContents.PullRequestURL)
statuses, err := db.GetChatDiffStatusesByChatID(dbauthz.AsSystemRestricted(ctx), chat.ID)
require.NoError(t, err)
require.Len(t, statuses, 1, "the discovery write must update the reported ref, not add a row")
require.Equal(t, "feature/keyed-discovery", statuses[0].GitBranch)
require.Equal(t, origin, statuses[0].GitRemoteOrigin)
require.True(t, statuses[0].Url.Valid)
require.Equal(t, prURL, statuses[0].Url.String)
})
t.Run("DiscoveryWriteDoesNotReorderPrimary", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
db, _ := dbtestutil.NewDB(t)
user := dbgen.User(t, db, database.User{})
org := dbgen.Organization(t, db, database.Organization{})
_ = dbgen.ChatProvider(t, db, database.ChatProvider{})
modelCfg := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{
Model: "test-model",
ContextLimit: 100000,
})
chat := dbgen.Chat(t, db, database.Chat{
OrganizationID: org.ID,
OwnerID: user.ID,
LastModelConfigID: modelCfg.ID,
Title: "discovery-write-primary-order",
})
// An old ref, then a newer ref reported later. The newer
// report is the primary.
_, err := db.UpsertChatDiffStatusReference(ctx, database.UpsertChatDiffStatusReferenceParams{
ChatID: chat.ID,
GitBranch: "old",
GitRemoteOrigin: "https://github.com/o/r",
StaleAt: time.Now().Add(-2 * time.Minute),
Url: sql.NullString{},
})
require.NoError(t, err)
_, err = db.UpsertChatDiffStatusReference(ctx, database.UpsertChatDiffStatusReferenceParams{
ChatID: chat.ID,
GitBranch: "new",
GitRemoteOrigin: "https://github.com/o/r",
StaleAt: time.Now().Add(-time.Minute),
Url: sql.NullString{},
})
require.NoError(t, err)
before, err := db.GetChatDiffStatusesByChatID(ctx, chat.ID)
require.NoError(t, err)
require.Len(t, before, 2)
require.Equal(t, "new", before[0].GitBranch)
// A discovery write lands on the old ref after the new one
// was reported, as an in-flight diff GET would.
err = db.UpdateChatDiffStatusReferenceURL(ctx, database.UpdateChatDiffStatusReferenceURLParams{
ChatID: chat.ID,
GitBranch: "old",
GitRemoteOrigin: "https://github.com/o/r",
StaleAt: time.Now().Add(-time.Minute),
Url: "https://github.com/o/r/pull/7",
})
require.NoError(t, err)
// The discovery write must not steal the primary position
// from the newer report.
after, err := db.GetChatDiffStatusesByChatID(ctx, chat.ID)
require.NoError(t, err)
require.Len(t, after, 2)
require.Equal(t, "new", after[0].GitBranch, "discovery must not reorder the primary")
require.Equal(t, "https://github.com/o/r/pull/7", after[1].Url.String)
})
t.Run("NotFoundForDifferentUser", func(t *testing.T) {
t.Parallel()
+7 -3
View File
@@ -3265,13 +3265,17 @@ func (p *Server) PublishDiffStatusChange(ctx context.Context, chatID uuid.UUID)
return xerrors.Errorf("get chat: %w", err)
}
dbStatus, err := p.db.GetChatDiffStatusByChatID(ctx, chatID)
dbStatuses, err := p.db.GetChatDiffStatusesByChatID(ctx, chatID)
if err != nil {
return xerrors.Errorf("get chat diff status: %w", err)
}
sdkStatus := db2sdk.ChatDiffStatus(chatID, &dbStatus)
p.publishChatPubsubEvent(chat, codersdk.ChatWatchEventKindDiffStatusChange, &sdkStatus)
var sdkStatus *codersdk.ChatDiffStatus
if len(dbStatuses) > 0 {
s := db2sdk.ChatDiffStatus(chatID, &dbStatuses[0])
sdkStatus = &s
}
p.publishChatPubsubEvent(chat, codersdk.ChatWatchEventKindDiffStatusChange, sdkStatus)
return nil
}
+4 -2
View File
@@ -335,8 +335,10 @@ func (r *Refresher) refreshOne(
now := r.clock.Now().UTC()
params := &database.UpsertChatDiffStatusParams{
ChatID: row.ChatID,
Url: sql.NullString{String: prURL, Valid: prURL != ""},
ChatID: row.ChatID,
GitRemoteOrigin: row.GitRemoteOrigin,
GitBranch: row.GitBranch,
Url: sql.NullString{String: prURL, Valid: prURL != ""},
PullRequestState: sql.NullString{
String: string(status.State),
Valid: status.State != "",
+29 -13
View File
@@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"errors"
"regexp"
"time"
"github.com/google/uuid"
@@ -206,7 +207,7 @@ func (w *Worker) tick(ctx context.Context) {
for _, res := range results {
if errors.Is(res.Error, ErrStalePullRequest) {
if err := w.clearStalePR(ctx, res.Request.Row.ChatID); err != nil {
if err := w.clearStalePR(ctx, res.Request.Row); err != nil {
w.logger.Warn(ctx, "clear stale chat diff status PR",
slog.F("chat_id", res.Request.Row.ChatID),
slog.Error(err))
@@ -227,8 +228,10 @@ func (w *Worker) tick(ctx context.Context) {
// Back off so the row isn't retried immediately.
if err := w.store.BackoffChatDiffStatus(ctx,
database.BackoffChatDiffStatusParams{
ChatID: res.Request.Row.ChatID,
StaleAt: w.clock.Now().UTC().Add(backoff),
ChatID: res.Request.Row.ChatID,
GitRemoteOrigin: res.Request.Row.GitRemoteOrigin,
GitBranch: res.Request.Row.GitBranch,
StaleAt: w.clock.Now().UTC().Add(backoff),
},
); err != nil {
w.logger.Warn(ctx, "backoff failed chat diff status",
@@ -249,8 +252,10 @@ func (w *Worker) tick(ctx context.Context) {
if age < NoPRRetryWindow {
if err := w.store.BackoffChatDiffStatus(ctx,
database.BackoffChatDiffStatusParams{
ChatID: res.Request.Row.ChatID,
StaleAt: w.clock.Now().UTC().Add(NoPRBackoff),
ChatID: res.Request.Row.ChatID,
GitRemoteOrigin: res.Request.Row.GitRemoteOrigin,
GitBranch: res.Request.Row.GitBranch,
StaleAt: w.clock.Now().UTC().Add(NoPRBackoff),
},
); err != nil {
w.logger.Warn(ctx, "backoff no-pr chat diff status",
@@ -296,6 +301,8 @@ func (w *Worker) MarkStale(ctx context.Context, p MarkStaleParams) {
return
}
p.Origin = originCredentials.ReplaceAllString(p.Origin, "$1")
// When a specific chat is identified, target it directly
// instead of broadcasting to every chat on the workspace.
// Note: this path does not verify that the chat belongs to
@@ -324,8 +331,14 @@ func (w *Worker) MarkStale(ctx context.Context, p MarkStaleParams) {
}
}
// markStaleSingle upserts the git ref for a single chat and
// publishes a diff-status change event.
// originCredentials matches the user and password in an HTTP(S) remote
// such as "https://<token>@github.com/o/r.git". SSH remotes keep their
// user: it is not a secret, and GitHub needs "git@" to parse them.
// Migration 000606 strips existing rows with the same pattern.
var originCredentials = regexp.MustCompile(`^(https?://)[^/]*@`)
// markStaleSingle upserts the git ref for a single chat and publishes
// a diff-status change event.
func (w *Worker) markStaleSingle(
ctx context.Context,
chatID uuid.UUID,
@@ -346,6 +359,7 @@ func (w *Worker) markStaleSingle(
slog.Error(err))
return
}
// Notify the frontend immediately so the UI shows the
// branch info even before the worker refreshes PR data.
if w.publishDiffStatusChangeFn != nil {
@@ -358,17 +372,19 @@ func (w *Worker) markStaleSingle(
// clearStalePR drops a chat's stored PR when its branch no longer
// has one.
func (w *Worker) clearStalePR(ctx context.Context, chatID uuid.UUID) error {
func (w *Worker) clearStalePR(ctx context.Context, row database.ChatDiffStatus) error {
if err := w.store.ClearChatDiffStatusPR(ctx, database.ClearChatDiffStatusPRParams{
ChatID: chatID,
StaleAt: w.clock.Now().UTC().Add(NoPRBackoff),
ChatID: row.ChatID,
GitRemoteOrigin: row.GitRemoteOrigin,
GitBranch: row.GitBranch,
StaleAt: w.clock.Now().UTC().Add(NoPRBackoff),
}); err != nil {
return xerrors.Errorf("clear stale chat diff status PR: %w", err)
}
if w.publishDiffStatusChangeFn != nil {
if err := w.publishDiffStatusChangeFn(ctx, chatID); err != nil {
if err := w.publishDiffStatusChangeFn(ctx, row.ChatID); err != nil {
w.logger.Debug(ctx, "publish diff status change",
slog.F("chat_id", chatID), slog.Error(err))
slog.F("chat_id", row.ChatID), slog.Error(err))
}
}
return nil
@@ -398,7 +414,7 @@ func (w *Worker) RefreshChat(
}
res := results[0]
if errors.Is(res.Error, ErrStalePullRequest) {
if err := w.clearStalePR(ctx, row.ChatID); err != nil {
if err := w.clearStalePR(ctx, row); err != nil {
return nil, err
}
return nil, nil
+112 -1
View File
@@ -812,6 +812,52 @@ func TestWorker_MarkStale_EmptyBranchOrOrigin(t *testing.T) {
}
}
func TestWorker_MarkStale_StripsOriginCredentials(t *testing.T) {
t.Parallel()
tests := []struct {
name string
origin string
want string
}{
{"token as user", "https://ghp_abc@github.com/o/r.git", "https://github.com/o/r.git"},
{"user and password", "http://oauth2:tok@git.example.com:8080/o/r.git", "http://git.example.com:8080/o/r.git"},
{"no credentials", "https://github.com/o/r.git", "https://github.com/o/r.git"},
{"at sign in path", "https://github.com/o/r@v1", "https://github.com/o/r@v1"},
{"ssh URL keeps user", "ssh://git@github.com/o/r.git", "ssh://git@github.com/o/r.git"},
{"scp-style ssh", "git@github.com:o/r.git", "git@github.com:o/r.git"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitShort)
ctrl := gomock.NewController(t)
store := dbmock.NewMockStore(ctrl)
var stored string
store.EXPECT().UpsertChatDiffStatusReference(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, arg database.UpsertChatDiffStatusReferenceParams) (database.ChatDiffStatus, error) {
stored = arg.GitRemoteOrigin
return database.ChatDiffStatus{ChatID: arg.ChatID}, nil
})
mClock := quartz.NewMock(t)
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
worker := gitsync.NewWorker(store, newTestRefresher(t, mClock), nil, mClock, logger)
worker.MarkStale(ctx, gitsync.MarkStaleParams{
WorkspaceID: uuid.New(),
Branch: "main",
Origin: tc.origin,
ChatID: uuid.New(),
})
require.Equal(t, tc.want, stored)
})
}
}
func TestWorker_MarkStale_WithChatID(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitShort)
@@ -993,8 +1039,10 @@ func TestWorker(t *testing.T) {
require.Equal(t, int32(1), publishCount.Load())
// 9. Read back and verify persisted fields.
status, err := db.GetChatDiffStatusByChatID(ctx, chat.ID)
statuses, err := db.GetChatDiffStatusesByChatID(ctx, chat.ID)
require.NoError(t, err)
require.Len(t, statuses, 1)
status := statuses[0]
// The mock resolveBranchPR returns PRRef{Owner: "o", Repo: "r", Number: 1}
// and buildPullRequestURL formats it as https://github.com/o/r/pull/1.
@@ -1011,6 +1059,69 @@ func TestWorker(t *testing.T) {
assert.WithinDuration(t, expectedStaleAt, status.StaleAt, time.Second)
}
func TestWorker_RefreshDoesNotReorderPrimary(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
// A real database, because the ordering contract is SQL.
db, _ := dbtestutil.NewDB(t)
user := dbgen.User(t, db, database.User{})
org := dbgen.Organization(t, db, database.Organization{})
_ = dbgen.ChatProvider(t, db, database.ChatProvider{})
modelCfg := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{
Model: "test-model",
ContextLimit: 100000,
})
chat := dbgen.Chat(t, db, database.Chat{
OrganizationID: org.ID,
OwnerID: user.ID,
LastModelConfigID: modelCfg.ID,
Title: "primary-order",
})
// An old ref with a PR, and a newer ref reported later. The newer
// report must stay primary even after the old ref refreshes.
_, err := db.UpsertChatDiffStatusReference(ctx, database.UpsertChatDiffStatusReferenceParams{
ChatID: chat.ID,
GitBranch: "old",
GitRemoteOrigin: "https://github.com/o/r",
StaleAt: time.Now().Add(-2 * time.Minute),
Url: sql.NullString{},
})
require.NoError(t, err)
_, err = db.UpsertChatDiffStatusReference(ctx, database.UpsertChatDiffStatusReferenceParams{
ChatID: chat.ID,
GitBranch: "new",
GitRemoteOrigin: "https://github.com/o/r",
StaleAt: time.Now().Add(-time.Minute),
Url: sql.NullString{},
})
require.NoError(t, err)
before, err := db.GetChatDiffStatusesByChatID(ctx, chat.ID)
require.NoError(t, err)
require.Len(t, before, 2)
require.Equal(t, "new", before[0].GitBranch)
// Refresh the old ref only. Both rows are stale, so acquire the
// old one explicitly through the sync path.
oldRow := before[1]
mClock := quartz.NewMock(t)
refresher := newTestRefresher(t, mClock)
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
worker := gitsync.NewWorker(db, refresher, nil, mClock, logger)
_, err = worker.RefreshChat(ctx, oldRow, user.ID)
require.NoError(t, err)
// The refreshed old ref must not steal the primary position from
// the newer report.
after, err := db.GetChatDiffStatusesByChatID(ctx, chat.ID)
require.NoError(t, err)
require.Len(t, after, 2)
require.Equal(t, "new", after[0].GitBranch, "refresh must not reorder the primary")
require.True(t, after[1].RefreshedAt.Valid, "the old ref was refreshed")
}
func TestRefreshChat_Success(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitShort)