From 40395c6e32d7d43a475d28235221e48c404c04f0 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Wed, 25 Mar 2026 10:09:44 -0400 Subject: [PATCH] fix(coderd): fast-retry PR discovery after git push (#23579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem When chatd pushes a branch and then creates a PR (e.g. `git push` followed by `gh pr create`), the gitsync background worker often picks up the stale `chat_diff_statuses` row between the two operations. At that point no PR exists yet, so the worker skips the row. However, the acquisition SQL locks the row for **5 minutes** (crash-recovery interval), creating a dead zone where the PR diff is invisible in the UI until the user manually navigates to the chat. ### Root cause 1. `git push` triggers `GIT_ASKPASS` → coderd external-auth handler → `MarkStale()` sets `stale_at = now - 1s` 2. Background worker acquires the row within ~10s, atomically bumps `stale_at = NOW() + 5 min` (crash-recovery lock) 3. Worker calls `ResolveBranchPullRequest` → no PR exists yet → returns `nil` → worker skips with `continue` 4. `gh pr create` completes moments later, but uses its own auth (not `GIT_ASKPASS`), so no second `MarkStale` fires 5. Row is locked for 5 minutes before the worker can retry Loading the chat works immediately because `GET /chats/{chat}` calls `resolveChatDiffStatus` synchronously, which discovers the PR inline. ## Fix When `ResolveBranchPullRequest` returns nil (no PR yet) **and** the row was recently marked stale (within 2 minutes), apply a short 15-second backoff via `BackoffChatDiffStatus` instead of letting the 5-minute acquisition lock stand. Outside the retry window, the worker skips the row as before — no indefinite fast-polling for branches that never receive a PR. To make the "recently marked stale" check work, `updated_at` is no longer overwritten by the acquisition and backoff SQL queries. This preserves it as a reliable "last externally changed" timestamp (set by `MarkStale` or a successful refresh). ### Behavior summary | Scenario | `updated_at` age | Backoff | Effective retry | |---|---|---|---| | Fresh push, no PR yet | < 2 min | 15s (`NoPRBackoff`) | ~15s | | Old row, no PR | ≥ 2 min | None (skip) | ~5 min (acquisition lock) | | Error (any age) | Any | 120s (`DiffStatusTTL`) | ~120s | | Success (any age) | Any | 120s (`DiffStatusTTL`) | ~120s | ## Changes - **`coderd/database/queries/chats.sql`** — Remove `updated_at = NOW()` from `AcquireStaleChatDiffStatuses` and `BackoffChatDiffStatus` - **`coderd/database/queries.sql.go`** — Regenerated - **`coderd/x/gitsync/worker.go`** — Add `NoPRBackoff` (15s) and `NoPRRetryWindow` (2 min) constants; apply short backoff only within the retry window - **`coderd/x/gitsync/worker_test.go`** — Add `TestWorker_NoPR_RecentMarkStale_BacksOffShort` and `TestWorker_NoPR_OldRow_Skips` --- coderd/database/queries.sql.go | 14 ++- coderd/database/queries/chats.sql | 14 ++- coderd/x/gitsync/worker.go | 39 ++++++- coderd/x/gitsync/worker_test.go | 162 ++++++++++++++++++++++++++++-- 4 files changed, 212 insertions(+), 17 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 6ea25cf4ad..d0b4fa6924 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3883,8 +3883,11 @@ 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. - stale_at = NOW() + INTERVAL '5 minutes', - updated_at = NOW() + -- 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 ( SELECT @@ -4005,8 +4008,11 @@ const backoffChatDiffStatus = `-- name: BackoffChatDiffStatus :exec UPDATE chat_diff_statuses SET - stale_at = $1::timestamptz, - updated_at = NOW() + -- 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 ` diff --git a/coderd/database/queries/chats.sql b/coderd/database/queries/chats.sql index 440eeddb70..5788f99858 100644 --- a/coderd/database/queries/chats.sql +++ b/coderd/database/queries/chats.sql @@ -543,8 +543,11 @@ 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. - stale_at = NOW() + INTERVAL '5 minutes', - updated_at = NOW() + -- 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 ( SELECT @@ -579,8 +582,11 @@ INNER JOIN UPDATE chat_diff_statuses SET - stale_at = @stale_at::timestamptz, - updated_at = NOW() + -- 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; diff --git a/coderd/x/gitsync/worker.go b/coderd/x/gitsync/worker.go index ea805da679..fd4f1bf1a6 100644 --- a/coderd/x/gitsync/worker.go +++ b/coderd/x/gitsync/worker.go @@ -32,6 +32,24 @@ const ( // than DiffStatusTTL because the user must manually link // their account before retrying is useful. NoTokenBackoff = 10 * time.Minute + + // NoPRBackoff is the backoff applied when a branch has no + // associated pull request yet. Kept short so that PRs created + // shortly after a push (e.g. via `gh pr create`) are + // discovered quickly instead of waiting for the 5-minute + // acquisition lock to expire. + NoPRBackoff = 15 * time.Second + + // NoPRRetryWindow is how long after MarkStale the worker + // applies the short NoPRBackoff. Outside this window the + // worker lets the 5-minute acquisition lock serve as the + // natural retry interval, avoiding indefinite fast-polling + // for branches that never receive a PR. + // + // Together with NoPRBackoff this bounds the number of + // GitHub API calls to ~NoPRRetryWindow/NoPRBackoff (≈8) + // per push. Keep both values in sync when adjusting. + NoPRRetryWindow = 2 * time.Minute ) // Store is the narrow DB interface the Worker needs. @@ -218,7 +236,26 @@ func (w *Worker) tick(ctx context.Context) { continue } if res.Params == nil { - // No PR yet — skip. + // No PR exists yet for this branch. If the row was + // recently marked stale (e.g. a git push just + // happened), apply a short backoff so the PR is + // discovered quickly once created. Outside the + // retry window, do not shorten the backoff; the + // 5-minute acquisition lock will serve as the retry + // interval instead. + age := w.clock.Now().Sub(res.Request.Row.UpdatedAt) + if age < NoPRRetryWindow { + if err := w.store.BackoffChatDiffStatus(ctx, + database.BackoffChatDiffStatusParams{ + ChatID: res.Request.Row.ChatID, + StaleAt: w.clock.Now().UTC().Add(NoPRBackoff), + }, + ); err != nil { + w.logger.Warn(ctx, "backoff no-pr chat diff status", + slog.F("chat_id", res.Request.Row.ChatID), + slog.Error(err)) + } + } continue } if _, err := w.store.UpsertChatDiffStatus(ctx, *res.Params); err != nil { diff --git a/coderd/x/gitsync/worker_test.go b/coderd/x/gitsync/worker_test.go index 11d412b9fe..1a7d9910cc 100644 --- a/coderd/x/gitsync/worker_test.go +++ b/coderd/x/gitsync/worker_test.go @@ -221,30 +221,81 @@ func TestWorker_LimitsToNRows(t *testing.T) { assert.Equal(t, int32(numRows), upsertCount.Load()) } -func TestWorker_RefresherReturnsNilNil_SkipsUpsert(t *testing.T) { +func TestWorker_NoPR_RecentMarkStale_BacksOffShort(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) chatID := uuid.New() ownerID := uuid.New() - // When the Refresher returns (nil, nil) the worker skips the - // upsert and publish. We signal tickDone from the refresher - // mock since that is the last operation before the tick - // returns. + // When the Refresher returns (nil, nil) AND the row was + // recently marked stale (updated_at within NoPRRetryWindow), + // the worker should call BackoffChatDiffStatus with NoPRBackoff + // so the row is retried quickly. tickDone := make(chan struct{}) ctrl := gomock.NewController(t) store := dbmock.NewMockStore(ctrl) - store.EXPECT().AcquireStaleChatDiffStatuses(gomock.Any(), gomock.Any()). - Return([]database.AcquireStaleChatDiffStatusesRow{makeAcquiredRowWithBranch(chatID, ownerID, "feature")}, nil) - mClock := quartz.NewMock(t) + + row := makeAcquiredRowWithBranch(chatID, ownerID, "feature") + row.UpdatedAt = mClock.Now() // recently marked stale + + store.EXPECT().AcquireStaleChatDiffStatuses(gomock.Any(), gomock.Any()). + Return([]database.AcquireStaleChatDiffStatusesRow{row}, nil) + store.EXPECT().BackoffChatDiffStatus(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, arg database.BackoffChatDiffStatusParams) error { + assert.Equal(t, chatID, arg.ChatID) + expected := mClock.Now().UTC().Add(gitsync.NoPRBackoff) + assert.WithinDuration(t, expected, arg.StaleAt, time.Second, + "stale_at should be NoPRBackoff from now") + close(tickDone) + return nil + }) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) // ResolveBranchPullRequest returns nil → Refresher returns // (nil, nil). + refresher := newTestRefresher(t, mClock, withResolveBranchPR( + func(context.Context, string, gitprovider.BranchRef) (*gitprovider.PRRef, error) { + return nil, nil + }, + )) + + worker := gitsync.NewWorker(store, refresher, nil, mClock, logger) + + tickOnce(ctx, t, mClock, worker, tickDone) +} + +func TestWorker_NoPR_OldRow_Skips(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chatID := uuid.New() + ownerID := uuid.New() + + // When the Refresher returns (nil, nil) but the row's + // updated_at is outside the NoPRRetryWindow, the worker should + // skip the row entirely (no backoff call) and let the 5-minute + // acquisition lock serve as the natural retry interval. + tickDone := make(chan struct{}) + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + + mClock := quartz.NewMock(t) + + row := makeAcquiredRowWithBranch(chatID, ownerID, "feature") + row.UpdatedAt = mClock.Now().Add(-5 * time.Minute) // old row + + store.EXPECT().AcquireStaleChatDiffStatuses(gomock.Any(), gomock.Any()). + Return([]database.AcquireStaleChatDiffStatusesRow{row}, nil) + // BackoffChatDiffStatus should NOT be called. + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + refresher := newTestRefresher(t, mClock, withResolveBranchPR( func(context.Context, string, gitprovider.BranchRef) (*gitprovider.PRRef, error) { close(tickDone) @@ -257,6 +308,101 @@ func TestWorker_RefresherReturnsNilNil_SkipsUpsert(t *testing.T) { tickOnce(ctx, t, mClock, worker, tickDone) } +func TestWorker_NoPR_BoundaryExactWindow_Skips(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chatID := uuid.New() + ownerID := uuid.New() + + // When updated_at is exactly NoPRRetryWindow ago, the strict + // "<" comparison means the row should be skipped (no backoff). + // This pins the boundary so an accidental change to "<=" is + // caught. + tickDone := make(chan struct{}) + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + + mClock := quartz.NewMock(t) + + row := makeAcquiredRowWithBranch(chatID, ownerID, "feature") + row.UpdatedAt = mClock.Now().Add(-gitsync.NoPRRetryWindow) // exactly at boundary + + store.EXPECT().AcquireStaleChatDiffStatuses(gomock.Any(), gomock.Any()). + Return([]database.AcquireStaleChatDiffStatusesRow{row}, nil) + // BackoffChatDiffStatus should NOT be called. + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + refresher := newTestRefresher(t, mClock, withResolveBranchPR( + func(context.Context, string, gitprovider.BranchRef) (*gitprovider.PRRef, error) { + close(tickDone) + return nil, nil + }, + )) + + worker := gitsync.NewWorker(store, refresher, nil, mClock, logger) + + tickOnce(ctx, t, mClock, worker, tickDone) +} + +func TestWorker_NoPR_BackoffError_ContinuesNextRow(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chat1 := uuid.New() + chat2 := uuid.New() + ownerID := uuid.New() + + // Two recent rows, both with no PR. BackoffChatDiffStatus + // fails for the first row but the second row should still + // be processed (backoff succeeds). + var backoffCount atomic.Int32 + tickDone := make(chan struct{}) + var closeOnce sync.Once + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + + mClock := quartz.NewMock(t) + + row1 := makeAcquiredRowWithBranch(chat1, ownerID, "no-pr-1") + row1.UpdatedAt = mClock.Now() + row2 := makeAcquiredRowWithBranch(chat2, ownerID, "no-pr-2") + row2.UpdatedAt = mClock.Now() + + store.EXPECT().AcquireStaleChatDiffStatuses(gomock.Any(), gomock.Any()). + Return([]database.AcquireStaleChatDiffStatusesRow{row1, row2}, nil) + store.EXPECT().BackoffChatDiffStatus(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, arg database.BackoffChatDiffStatusParams) error { + n := backoffCount.Add(1) + if arg.ChatID == chat1 { + return fmt.Errorf("simulated backoff error") + } + // Second call succeeds; both rows processed. + if n >= 2 { + closeOnce.Do(func() { close(tickDone) }) + } + return nil + }).Times(2) + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + refresher := newTestRefresher(t, mClock, withResolveBranchPR( + func(context.Context, string, gitprovider.BranchRef) (*gitprovider.PRRef, error) { + return nil, nil + }, + )) + + worker := gitsync.NewWorker(store, refresher, nil, mClock, logger) + + tickOnce(ctx, t, mClock, worker, tickDone) + + assert.Equal(t, int32(2), backoffCount.Load(), + "both rows should have attempted backoff") +} + func TestWorker_RefresherError_BacksOffRow(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort)