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"},