From faffb53021d68de03071e8dd1546befbf50f8d25 Mon Sep 17 00:00:00 2001 From: puppe1990 Date: Thu, 10 Sep 2026 16:02:46 -0300 Subject: [PATCH] Stop traffic jobs from exhausting the GitHub token budget Production piled up 657 failed SnapshotTraffic jobs, all with "github: rate limited", each burning its 3 attempts within seconds. Three compounding causes: - withTraffic enqueued a SnapshotTraffic job on every page view, with no dedup. Each job snapshots every cached repo (307 user + 114 org repos, two API calls each, ~842 requests), so a handful of page views spent the whole 5000/hour token budget. - SnapshotLogin kept walking the remaining ~420 repos after the first rate-limit error, spending ~45s per attempt firing requests that could only fail. GitHub warns this can get an integration banned. - The worker backs off 2s/4s/8s while GitHub's window resets in up to an hour, so the job was marked failed for good before the quota returned. Traffic refresh already runs on the hourly SnapshotTraffic cron, so the per-view enqueue was redundant: the request path now only fills repos that have no snapshot yet. A rate limit ends the run without marking it failed, leaving the next scheduled run to refresh it. Co-authored-by: CommandCodeBot --- internal/catalog/load.go | 35 +++++++++--------- internal/catalog/load_test.go | 44 +++++++++++++++++++++++ internal/jobs/snapshot_traffic.go | 50 +++++++++++++++++--------- internal/jobs/snapshot_traffic_test.go | 11 ++++++ 4 files changed, 105 insertions(+), 35 deletions(-) diff --git a/internal/catalog/load.go b/internal/catalog/load.go index 8c10396..324c9eb 100644 --- a/internal/catalog/load.go +++ b/internal/catalog/load.go @@ -49,13 +49,13 @@ func (l *Loader) User(ctx context.Context, login string) (Snapshot, error) { if snap, ok, err := l.cachedUser(login); err != nil { return Snapshot{}, err } else if ok { - return l.withTraffic(ctx, login, snap) + return l.withTraffic(ctx, snap) } } snap, err := l.fetchUser(ctx, login) if err == nil { - return l.withTraffic(ctx, login, snap) + return l.withTraffic(ctx, snap) } if errors.Is(err, githubapi.ErrNotFound) { return Snapshot{}, err @@ -123,7 +123,7 @@ func (l *Loader) All(ctx context.Context, login string) (Snapshot, error) { Source: SourceAll, SourceLogin: login, } - return l.withTraffic(ctx, login, snap) + return l.withTraffic(ctx, snap) } func (l *Loader) Org(ctx context.Context, userLogin, orgLogin string) (Snapshot, error) { @@ -143,7 +143,7 @@ func (l *Loader) Org(ctx context.Context, userLogin, orgLogin string) (Snapshot, if snapErr != nil { return Snapshot{}, snapErr } - return l.withTraffic(ctx, orgLogin, snap) + return l.withTraffic(ctx, snap) } return Snapshot{}, err } @@ -155,7 +155,7 @@ func (l *Loader) Org(ctx context.Context, userLogin, orgLogin string) (Snapshot, if err != nil { return Snapshot{}, err } - return l.withTraffic(ctx, orgLogin, snap) + return l.withTraffic(ctx, snap) } func (l *Loader) Refresh(ctx context.Context, login string) error { @@ -189,27 +189,23 @@ func (l *Loader) SnapshotLogin(ctx context.Context, login string) error { if err != nil { return err } - var first error for _, repo := range repos { - if err := l.SnapshotTraffic(ctx, repo.OwnerLogin, repo.Name); err != nil && first == nil { - first = err + if err := l.SnapshotTraffic(ctx, repo.OwnerLogin, repo.Name); err != nil { + return err } } for _, org := range orgs { orgRepos, err := l.Cache.LoadRepos(org.Login, SourceOrg) if err != nil { - if first == nil { - first = err - } - continue + return err } for _, repo := range orgRepos { - if err := l.SnapshotTraffic(ctx, repo.OwnerLogin, repo.Name); err != nil && first == nil { - first = err + if err := l.SnapshotTraffic(ctx, repo.OwnerLogin, repo.Name); err != nil { + return err } } } - return first + return nil } func (l *Loader) fetchUser(ctx context.Context, login string) (Snapshot, error) { @@ -295,14 +291,15 @@ func (l *Loader) orgSnapshot(userSnap Snapshot, orgLogin string, repos []Repo) ( }, nil } -func (l *Loader) withTraffic(ctx context.Context, login string, snap Snapshot) (Snapshot, error) { +// withTraffic refreshes traffic only for repos that have no snapshot yet. +// Refreshing cached repos is the hourly SnapshotTraffic cron's job: enqueueing +// it per request snapshots every cached repo (two API calls each) and burns the +// whole GitHub token budget within a handful of page views. +func (l *Loader) withTraffic(ctx context.Context, snap Snapshot) (Snapshot, error) { if l.GitHub.HasToken() { if err := l.fillMissingTraffic(ctx, snap.Repos); err != nil { return Snapshot{}, err } - if l.Queue != nil { - _ = l.Queue.EnqueueTraffic(login) - } } repos, err := l.attachTraffic(snap.Repos) if err != nil { diff --git a/internal/catalog/load_test.go b/internal/catalog/load_test.go index 07c3c5a..59d0be6 100644 --- a/internal/catalog/load_test.go +++ b/internal/catalog/load_test.go @@ -3,6 +3,7 @@ package catalog import ( "context" "errors" + "fmt" "strings" "testing" "time" @@ -21,6 +22,7 @@ type fakeGitHub struct { trafficErr error token bool users int + trafficN int } func (f *fakeGitHub) HasToken() bool { return f.token } @@ -43,6 +45,7 @@ func (f *fakeGitHub) OrgRepos(context.Context, string) ([]githubapi.Repo, error) return f.orgRepos, nil } func (f *fakeGitHub) Traffic(context.Context, string, string) (githubapi.Traffic, error) { + f.trafficN++ return f.traffic, f.trafficErr } @@ -255,6 +258,47 @@ func TestLoader_SnapshotTraffic_Persists(t *testing.T) { } } +func TestLoader_User_DoesNotEnqueueTrafficPerView(t *testing.T) { + gh := &fakeGitHub{ + token: true, + user: githubapi.User{Login: "puppe1990", Name: "Matheus"}, + repos: []githubapi.Repo{{Name: "cais", OwnerLogin: "puppe1990"}}, + traffic: githubapi.Traffic{Views: 17, Available: true}, + } + q := &fakeQueue{} + loader, _ := testLoader(t, gh, q) + + for i := 0; i < 3; i++ { + if _, err := loader.User(context.Background(), "puppe1990"); err != nil { + t.Fatal(err) + } + } + if len(q.traffic) != 0 { + t.Fatalf("page views enqueued %d traffic job(s): %v", len(q.traffic), q.traffic) + } +} + +func TestLoader_SnapshotLogin_StopsOnFirstRateLimit(t *testing.T) { + gh := &fakeGitHub{trafficErr: githubapi.ErrRateLimited} + loader, s := testLoader(t, gh, &fakeQueue{}) + + repos := make([]Repo, 0, 25) + for i := 0; i < 25; i++ { + repos = append(repos, Repo{Name: fmt.Sprintf("repo-%02d", i), OwnerLogin: "puppe1990"}) + } + if err := s.SaveRepos("puppe1990", SourceUser, repos, time.Now()); err != nil { + t.Fatal(err) + } + + err := loader.SnapshotLogin(context.Background(), "puppe1990") + if !errors.Is(err, githubapi.ErrRateLimited) { + t.Fatalf("err = %v, want ErrRateLimited", err) + } + if gh.trafficN != 1 { + t.Fatalf("traffic calls = %d, want 1 (stop at the first rate limit)", gh.trafficN) + } +} + func TestLoader_User_SkipsCacheWhenAuthenticatedSelf(t *testing.T) { gh := &fakeGitHub{ token: true, diff --git a/internal/jobs/snapshot_traffic.go b/internal/jobs/snapshot_traffic.go index e28273c..bd98123 100644 --- a/internal/jobs/snapshot_traffic.go +++ b/internal/jobs/snapshot_traffic.go @@ -3,10 +3,13 @@ package jobs import ( "context" "encoding/json" + "errors" + "log" caisjobs "github.com/puppe1990/cais/pkg/cais/jobs" "github.com/puppe1990/github-projects-viewer-cais/internal/catalog" + "github.com/puppe1990/github-projects-viewer-cais/internal/githubapi" ) type trafficPayload struct { @@ -19,22 +22,37 @@ func PerformSnapshotTraffic(loader *catalog.Loader) caisjobs.Handler { return func(ctx context.Context, payload []byte) error { var p trafficPayload _ = json.Unmarshal(payload, &p) - if p.Owner != "" && p.Repo != "" { - return loader.SnapshotTraffic(ctx, p.Owner, p.Repo) - } - if p.Login != "" { - return loader.SnapshotLogin(ctx, p.Login) - } - logins, err := loader.Cache.WatchedLogins() - if err != nil { - return err - } - var first error - for _, login := range logins { - if err := loader.SnapshotLogin(ctx, login); err != nil && first == nil { - first = err - } + return skipRateLimited(snapshotTraffic(ctx, loader, p)) + } +} + +func snapshotTraffic(ctx context.Context, loader *catalog.Loader, p trafficPayload) error { + if p.Owner != "" && p.Repo != "" { + return loader.SnapshotTraffic(ctx, p.Owner, p.Repo) + } + if p.Login != "" { + return loader.SnapshotLogin(ctx, p.Login) + } + logins, err := loader.Cache.WatchedLogins() + if err != nil { + return err + } + var first error + for _, login := range logins { + if err := loader.SnapshotLogin(ctx, login); err != nil && first == nil { + first = err } - return first } + return first +} + +// skipRateLimited ends a rate-limited run without marking it failed. The worker +// backs off seconds while GitHub's window resets in up to an hour, so retrying +// now only buries the job in the failed pile; the next scheduled run refreshes it. +func skipRateLimited(err error) error { + if !errors.Is(err, githubapi.ErrRateLimited) { + return err + } + log.Printf("jobs skipped: %v; the hourly SnapshotTraffic cron retries later", err) + return nil } diff --git a/internal/jobs/snapshot_traffic_test.go b/internal/jobs/snapshot_traffic_test.go index c55b1e0..6d75690 100644 --- a/internal/jobs/snapshot_traffic_test.go +++ b/internal/jobs/snapshot_traffic_test.go @@ -83,6 +83,17 @@ func TestPerformSnapshotTraffic_LoginUsesCachedRepos(t *testing.T) { } } +func TestPerformSnapshotTraffic_RateLimitedIsSkipped(t *testing.T) { + loader, s := testJobLoader(t, stubGitHub{trafficErr: githubapi.ErrRateLimited}) + if err := s.SaveRepos("octocat", catalog.SourceUser, []catalog.Repo{{Name: "hello-world", OwnerLogin: "octocat"}}, time.Now()); err != nil { + t.Fatal(err) + } + h := PerformSnapshotTraffic(loader) + if err := h(context.Background(), []byte(`{"login":"octocat"}`)); err != nil { + t.Fatalf("rate limit must not fail the job: %v", err) + } +} + func TestPerformRefreshCatalog_FetchesUser(t *testing.T) { loader, s := testJobLoader(t, stubGitHub{ user: githubapi.User{Login: "octocat", Name: "The Octocat"},