mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/gitsync): consolidate chat diff refresh paths through Worker.RefreshChat (#22938)
## Problem
Two separate code paths refreshed chat diff statuses:
1. **HTTP handler** (`refreshChatDiffStatus`): resolved
provider/token/status inline, ran under the user's context. Worked fine
because the user owns their external auth links.
2. **Background worker** (`Refresher.Refresh`): ran under `AsChatd`
context, which lacked `ActionReadPersonal` on `ResourceUser`.
`GetExternalAuthLink` failed silently (`if err != nil { continue }`),
returning `ErrNoTokenAvailable` every time. Chat diff statuses got
`git_branch`/`git_remote_origin` from `MarkStale` but `refreshed_at`,
`url`, `pull_request_state` stayed nil.
Having two paths also meant bug fixes had to be applied twice.
## Fix
- **`Worker.RefreshChat`**: New method for synchronous, on-demand
refresh of a single chat. Uses the same `Refresher.Refresh` pipeline as
the background `tick()`. Called by the HTTP handler for instant
response.
- **`resolveChatGitAccessToken`**: Uses
`dbauthz.AsSystemRestricted(ctx)` specifically for `GetExternalAuthLink`
and `RefreshToken` calls. This is scoped to just those DB operations
rather than broadening the chatd RBAC role.
- **Removed**: `refreshChatDiffStatus`, `shouldRefreshChatDiffStatus`,
`resolveChatDiffStatusWithOptions` (all replaced by the single
`RefreshChat` path).
## Tests
Added 4 tests for `Worker.RefreshChat`:
- `TestRefreshChat_Success`: full refresh + upsert + publish
- `TestRefreshChat_NoPR`: no PR exists yet, nil result
- `TestRefreshChat_RefreshError`: provider resolution fails
- `TestRefreshChat_UpsertError`: refresh succeeds but DB write fails
## Why tests didn't catch the original bug
- Worker tests used mock stores (no dbauthz) and fake token resolvers
(hardcoded lambdas)
- No integration test exercised `AsChatd` -> `resolveChatGitAccessToken`
-> `GetExternalAuthLink` through dbauthz
This commit is contained in:
@@ -6,6 +6,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
@@ -238,6 +239,52 @@ func (w *Worker) MarkStale(
|
||||
}
|
||||
}
|
||||
|
||||
// RefreshChat synchronously refreshes a single chat's diff
|
||||
// status using the same Refresher pipeline as the background
|
||||
// worker. Returns nil, nil when no PR exists yet for the
|
||||
// branch. Called from HTTP handlers for instant feedback.
|
||||
func (w *Worker) RefreshChat(
|
||||
ctx context.Context,
|
||||
row database.ChatDiffStatus,
|
||||
ownerID uuid.UUID,
|
||||
) (*database.ChatDiffStatus, error) {
|
||||
requests := []RefreshRequest{{
|
||||
Row: row,
|
||||
OwnerID: ownerID,
|
||||
}}
|
||||
|
||||
results, err := w.refresher.Refresh(ctx, requests)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("refresh chat diff status: %w", err)
|
||||
}
|
||||
|
||||
if len(results) == 0 {
|
||||
return nil, nil
|
||||
}
|
||||
res := results[0]
|
||||
if res.Error != nil {
|
||||
return nil, xerrors.Errorf("refresh chat diff status: %w", res.Error)
|
||||
}
|
||||
if res.Params == nil {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
upserted, err := w.store.UpsertChatDiffStatus(ctx, *res.Params)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("upsert chat diff status: %w", err)
|
||||
}
|
||||
|
||||
if w.publishDiffStatusChangeFn != nil {
|
||||
if err := w.publishDiffStatusChangeFn(ctx, row.ChatID); err != nil {
|
||||
w.logger.Debug(ctx, "publish diff status change",
|
||||
slog.F("chat_id", row.ChatID),
|
||||
slog.Error(err))
|
||||
}
|
||||
}
|
||||
|
||||
return &upserted, nil
|
||||
}
|
||||
|
||||
// filterChatsByWorkspaceID returns only chats associated with
|
||||
// the given workspace.
|
||||
func filterChatsByWorkspaceID(
|
||||
|
||||
@@ -742,3 +742,164 @@ func TestWorker(t *testing.T) {
|
||||
expectedStaleAt := mClock.Now().Add(gitsync.DiffStatusTTL)
|
||||
assert.WithinDuration(t, expectedStaleAt, status.StaleAt, time.Second)
|
||||
}
|
||||
|
||||
func TestRefreshChat_Success(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
chatID := uuid.New()
|
||||
ownerID := uuid.New()
|
||||
|
||||
row := database.ChatDiffStatus{
|
||||
ChatID: chatID,
|
||||
GitBranch: "feature",
|
||||
GitRemoteOrigin: "https://github.com/owner/repo",
|
||||
}
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
store := dbmock.NewMockStore(ctrl)
|
||||
|
||||
upsertedStatus := database.ChatDiffStatus{
|
||||
ChatID: chatID,
|
||||
Url: sql.NullString{String: "https://github.com/o/r/pull/1", Valid: true},
|
||||
Additions: 10,
|
||||
Deletions: 3,
|
||||
ChangedFiles: 2,
|
||||
}
|
||||
store.EXPECT().UpsertChatDiffStatus(gomock.Any(), gomock.Any()).
|
||||
DoAndReturn(func(_ context.Context, arg database.UpsertChatDiffStatusParams) (database.ChatDiffStatus, error) {
|
||||
assert.Equal(t, chatID, arg.ChatID)
|
||||
return upsertedStatus, nil
|
||||
})
|
||||
|
||||
var publishCalled atomic.Bool
|
||||
pub := func(_ context.Context, id uuid.UUID) error {
|
||||
assert.Equal(t, chatID, id)
|
||||
publishCalled.Store(true)
|
||||
return nil
|
||||
}
|
||||
|
||||
mClock := quartz.NewMock(t)
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
|
||||
refresher := newTestRefresher(t, mClock)
|
||||
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
|
||||
|
||||
result, err := worker.RefreshChat(ctx, row, ownerID)
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, result)
|
||||
assert.Equal(t, chatID, result.ChatID)
|
||||
assert.Equal(t, upsertedStatus.Url, result.Url)
|
||||
assert.True(t, publishCalled.Load(), "publish should have been called")
|
||||
}
|
||||
|
||||
func TestRefreshChat_NoPR(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
chatID := uuid.New()
|
||||
ownerID := uuid.New()
|
||||
|
||||
row := database.ChatDiffStatus{
|
||||
ChatID: chatID,
|
||||
GitBranch: "feature",
|
||||
GitRemoteOrigin: "https://github.com/owner/repo",
|
||||
}
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
store := dbmock.NewMockStore(ctrl)
|
||||
// UpsertChatDiffStatus should NOT be called.
|
||||
|
||||
var publishCalled atomic.Bool
|
||||
pub := func(_ context.Context, _ uuid.UUID) error {
|
||||
publishCalled.Store(true)
|
||||
return nil
|
||||
}
|
||||
|
||||
mClock := quartz.NewMock(t)
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
|
||||
|
||||
// ResolveBranchPullRequest returns nil → no PR exists yet.
|
||||
refresher := newTestRefresher(t, mClock, withResolveBranchPR(
|
||||
func(context.Context, string, gitprovider.BranchRef) (*gitprovider.PRRef, error) {
|
||||
return nil, nil
|
||||
},
|
||||
))
|
||||
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
|
||||
|
||||
result, err := worker.RefreshChat(ctx, row, ownerID)
|
||||
require.NoError(t, err)
|
||||
assert.Nil(t, result, "result should be nil when no PR exists")
|
||||
assert.False(t, publishCalled.Load(), "publish should not be called when no PR exists")
|
||||
}
|
||||
|
||||
func TestRefreshChat_RefreshError(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
chatID := uuid.New()
|
||||
ownerID := uuid.New()
|
||||
|
||||
row := database.ChatDiffStatus{
|
||||
ChatID: chatID,
|
||||
Url: sql.NullString{String: "https://github.com/org/repo/pull/1", Valid: true},
|
||||
GitBranch: "feature",
|
||||
GitRemoteOrigin: "https://github.com/owner/repo",
|
||||
}
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
store := dbmock.NewMockStore(ctrl)
|
||||
// UpsertChatDiffStatus should NOT be called.
|
||||
|
||||
// Provider resolver returns nil → "no provider" error.
|
||||
providers := func(string) gitprovider.Provider { return nil }
|
||||
tokens := func(context.Context, uuid.UUID, string) (*string, error) {
|
||||
return ptr.Ref("tok"), nil
|
||||
}
|
||||
|
||||
mClock := quartz.NewMock(t)
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
|
||||
refresher := gitsync.NewRefresher(providers, tokens, logger, mClock)
|
||||
worker := gitsync.NewWorker(store, refresher, nil, mClock, logger)
|
||||
|
||||
result, err := worker.RefreshChat(ctx, row, ownerID)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "no provider")
|
||||
assert.Nil(t, result)
|
||||
}
|
||||
|
||||
func TestRefreshChat_UpsertError(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
chatID := uuid.New()
|
||||
ownerID := uuid.New()
|
||||
|
||||
row := database.ChatDiffStatus{
|
||||
ChatID: chatID,
|
||||
GitBranch: "feature",
|
||||
GitRemoteOrigin: "https://github.com/owner/repo",
|
||||
}
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
store := dbmock.NewMockStore(ctrl)
|
||||
|
||||
store.EXPECT().UpsertChatDiffStatus(gomock.Any(), gomock.Any()).
|
||||
Return(database.ChatDiffStatus{}, fmt.Errorf("db write error"))
|
||||
|
||||
var publishCalled atomic.Bool
|
||||
pub := func(_ context.Context, _ uuid.UUID) error {
|
||||
publishCalled.Store(true)
|
||||
return nil
|
||||
}
|
||||
|
||||
mClock := quartz.NewMock(t)
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
|
||||
refresher := newTestRefresher(t, mClock)
|
||||
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
|
||||
|
||||
result, err := worker.RefreshChat(ctx, row, ownerID)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "upsert chat diff status")
|
||||
assert.Nil(t, result)
|
||||
assert.False(t, publishCalled.Load(), "publish should not be called when upsert fails")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user