From 2994636bbc75467df9e160dd2d81974d830ce326 Mon Sep 17 00:00:00 2001 From: Petri-Johan Last Date: Fri, 19 Jul 2024 18:45:16 +0200 Subject: [PATCH] [update] Remove sleep in goroutine for webhook handlers (#63940) Since permission sync jobs are database backed, we no longer need to sleep in the webhook handler goroutines, since the permission sync jobs have a sleep time themselves. ## Test plan Tests still pass, no real functional changes. ## Changelog --- .../internal/authz/webhooks/github.go | 40 +++++++------------ .../internal/authz/webhooks/github_test.go | 4 +- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/cmd/frontend/internal/authz/webhooks/github.go b/cmd/frontend/internal/authz/webhooks/github.go index f3025f1dcc9..19dc0c1d015 100644 --- a/cmd/frontend/internal/authz/webhooks/github.go +++ b/cmd/frontend/internal/authz/webhooks/github.go @@ -53,31 +53,21 @@ func TestSetGitHubHandlerSleepTime(t *testing.T, val time.Duration) { sleepTime = val } -func (h *GitHubWebhook) handleGitHubWebhook(_ context.Context, db database.DB, codeHostURN extsvc.CodeHostBaseURL, payload any) error { - // TODO: This MUST be removed once permissions syncing jobs are database backed! - // If we react too quickly to a webhook, the changes may not yet have properly - // propagated on GitHub's system, and we'll get old results, making the - // webhook useless. - // We have to wait some amount of time to process the webhook to ensure - // that we are getting fresh results. - go func() { - time.Sleep(sleepTime) - eventContext, cancel := context.WithTimeout(context.Background(), 1*time.Minute) - defer cancel() - - switch e := payload.(type) { - case *gh.RepositoryEvent: - _ = h.handleRepositoryEvent(eventContext, db, e) - case *gh.MemberEvent: - _ = h.handleMemberEvent(eventContext, db, e, codeHostURN) - case *gh.OrganizationEvent: - _ = h.handleOrganizationEvent(eventContext, db, e, codeHostURN) - case *gh.MembershipEvent: - _ = h.handleMembershipEvent(eventContext, db, e, codeHostURN) - case *gh.TeamEvent: - _ = h.handleTeamEvent(eventContext, e, db) - } - }() +func (h *GitHubWebhook) handleGitHubWebhook(ctx context.Context, db database.DB, codeHostURN extsvc.CodeHostBaseURL, payload any) error { + // TODO: Should we return errors here? We could receive a webhook for a repo + // that's not on Sourcegraph, so probably not? + switch e := payload.(type) { + case *gh.RepositoryEvent: + _ = h.handleRepositoryEvent(ctx, db, e) + case *gh.MemberEvent: + _ = h.handleMemberEvent(ctx, db, e, codeHostURN) + case *gh.OrganizationEvent: + _ = h.handleOrganizationEvent(ctx, db, e, codeHostURN) + case *gh.MembershipEvent: + _ = h.handleMembershipEvent(ctx, db, e, codeHostURN) + case *gh.TeamEvent: + _ = h.handleTeamEvent(ctx, e, db) + } return nil } diff --git a/cmd/frontend/internal/authz/webhooks/github_test.go b/cmd/frontend/internal/authz/webhooks/github_test.go index b23081bf5e2..c5d6558afef 100644 --- a/cmd/frontend/internal/authz/webhooks/github_test.go +++ b/cmd/frontend/internal/authz/webhooks/github_test.go @@ -273,7 +273,9 @@ func TestGitHubWebhooks(t *testing.T) { req := newReq(t, webhookTest.eventType, webhookTest.event) responseRecorder := httptest.NewRecorder() - hook.ServeHTTP(responseRecorder, req) + go func() { + hook.ServeHTTP(responseRecorder, req) + }() waitUntil(t, webhookCalled) }) }