Skip to content

Commit 919500a

Browse files
committed
perf(github): cache check runs with sync.Once
Extract fetchAllCheckRunPages to collect all paginated check runs in one pass, then cache the result on Provider via sync.Once. Concurrent getExistingCheckRunID calls for multiple matched PipelineRuns share a single ListCheckRunsForRef API round-trip instead of each fetching independently. Also extract searchCheckRuns helper and adds a nil guard on ExternalID. Signed-off-by: Akshay Pant <akpant@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
1 parent 3c89639 commit 919500a

File tree

3 files changed

+105
-14
lines changed

3 files changed

+105
-14
lines changed

pkg/provider/github/github.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,20 @@ type Provider struct {
6767
pacUserLogin string // user/bot login used by PAC
6868
clock clockwork.Clock
6969
graphQLClient *graphQLClient
70+
checkRunsCache checkRunsCache
7071
}
7172

7273
type skippedRun struct {
7374
mutex *sync.Mutex
7475
checkRunID int64
7576
}
7677

78+
type checkRunsCache struct {
79+
once sync.Once
80+
runs []*github.CheckRun
81+
err error
82+
}
83+
7784
func New() *Provider {
7885
return &Provider{
7986
APIURL: github.Ptr(keys.PublicGithubAPIURL),

pkg/provider/github/status.go

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ const taskStatusTemplate = `
4242
{{- end }}
4343
</table>`
4444

45-
func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Event, status providerstatus.StatusOpts) (*int64, error) {
45+
// fetchAllCheckRunPages retrieves every page of check runs for the event SHA.
46+
func (v *Provider) fetchAllCheckRunPages(ctx context.Context, runevent *info.Event) ([]*github.CheckRun, error) {
47+
var all []*github.CheckRun
4648
opt := github.ListOptions{PerPage: v.PaginedNumber}
4749
for {
4850
res, resp, err := wrapAPI(v, "list_check_runs_for_ref", func() (*github.ListCheckRunsResults, *github.Response, error) {
@@ -55,25 +57,37 @@ func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Eve
5557
if err != nil {
5658
return nil, err
5759
}
58-
59-
for _, checkrun := range res.CheckRuns {
60-
// if it is a Pending approval CheckRun then overwrite it
61-
if isPendingApprovalCheckrun(checkrun) || isFailedCheckrun(checkrun) {
62-
if v.canIUseCheckrunID(checkrun.ID) {
63-
return checkrun.ID, nil
64-
}
65-
}
66-
if *checkrun.ExternalID == status.PipelineRunName {
67-
return checkrun.ID, nil
68-
}
69-
}
60+
all = append(all, res.CheckRuns...)
7061
if resp.NextPage == 0 {
7162
break
7263
}
7364
opt.Page = resp.NextPage
7465
}
66+
return all, nil
67+
}
7568

76-
return nil, nil
69+
func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Event, status providerstatus.StatusOpts) (*int64, error) {
70+
v.checkRunsCache.once.Do(func() {
71+
v.checkRunsCache.runs, v.checkRunsCache.err = v.fetchAllCheckRunPages(ctx, runevent)
72+
})
73+
if v.checkRunsCache.err != nil {
74+
return nil, v.checkRunsCache.err
75+
}
76+
return searchCheckRuns(v.checkRunsCache.runs, status, v), nil
77+
}
78+
79+
func searchCheckRuns(runs []*github.CheckRun, status providerstatus.StatusOpts, v *Provider) *int64 {
80+
for _, checkrun := range runs {
81+
if isPendingApprovalCheckrun(checkrun) || isFailedCheckrun(checkrun) {
82+
if v.canIUseCheckrunID(checkrun.ID) {
83+
return checkrun.ID
84+
}
85+
}
86+
if checkrun.ExternalID != nil && *checkrun.ExternalID == status.PipelineRunName {
87+
return checkrun.ID
88+
}
89+
}
90+
return nil
7791
}
7892

7993
func isPendingApprovalCheckrun(run *github.CheckRun) bool {

pkg/provider/github/status_test.go

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"reflect"
99
"strconv"
1010
"strings"
11+
"sync"
12+
"sync/atomic"
1113
"testing"
1214

1315
"github.com/google/go-github/v84/github"
@@ -686,3 +688,71 @@ func TestProviderGetExistingCheckRunID(t *testing.T) {
686688
})
687689
}
688690
}
691+
692+
func TestGetExistingCheckRunIDCacheHit(t *testing.T) {
693+
ctx, _ := rtesting.SetupFakeContext(t)
694+
client, mux, _, teardown := ghtesthelper.SetupGH()
695+
defer teardown()
696+
697+
event := &info.Event{
698+
Organization: "owner",
699+
Repository: "repository",
700+
SHA: "sha",
701+
}
702+
703+
apiHits := 0
704+
mux.HandleFunc(fmt.Sprintf("/repos/%v/%v/commits/%v/check-runs", event.Organization, event.Repository, event.SHA), func(w http.ResponseWriter, _ *http.Request) {
705+
apiHits++
706+
fmt.Fprint(w, `{"total_count": 1, "check_runs": [{"id": 55555, "external_id": "mypr"}]}`)
707+
})
708+
709+
cnx := New()
710+
cnx.SetGithubClient(client)
711+
712+
// First call — should hit the API and populate the cache.
713+
id, err := cnx.getExistingCheckRunID(ctx, event, providerstatus.StatusOpts{PipelineRunName: "mypr"})
714+
assert.NilError(t, err)
715+
assert.Assert(t, id != nil)
716+
717+
// Second call — should serve from cache, no additional API call.
718+
id2, err := cnx.getExistingCheckRunID(ctx, event, providerstatus.StatusOpts{PipelineRunName: "mypr"})
719+
assert.NilError(t, err)
720+
assert.Assert(t, id2 != nil)
721+
assert.Equal(t, *id, *id2)
722+
assert.Equal(t, apiHits, 1)
723+
}
724+
725+
func TestGetExistingCheckRunIDConcurrent(t *testing.T) {
726+
ctx, _ := rtesting.SetupFakeContext(t)
727+
client, mux, _, teardown := ghtesthelper.SetupGH()
728+
defer teardown()
729+
730+
event := &info.Event{
731+
Organization: "owner",
732+
Repository: "repository",
733+
SHA: "sha",
734+
}
735+
736+
var apiHits atomic.Int64
737+
mux.HandleFunc(fmt.Sprintf("/repos/%v/%v/commits/%v/check-runs", event.Organization, event.Repository, event.SHA), func(w http.ResponseWriter, _ *http.Request) {
738+
apiHits.Add(1)
739+
fmt.Fprint(w, `{"total_count": 1, "check_runs": [{"id": 55555, "external_id": "mypr"}]}`)
740+
})
741+
742+
cnx := New()
743+
cnx.SetGithubClient(client)
744+
745+
const goroutines = 10
746+
var wg sync.WaitGroup
747+
wg.Add(goroutines)
748+
for range goroutines {
749+
go func() {
750+
defer wg.Done()
751+
_, _ = cnx.getExistingCheckRunID(ctx, event, providerstatus.StatusOpts{PipelineRunName: "mypr"})
752+
}()
753+
}
754+
wg.Wait()
755+
756+
// Only one goroutine should have fetched from the API; the rest hit the cache.
757+
assert.Equal(t, apiHits.Load(), int64(1))
758+
}

0 commit comments

Comments
 (0)