feat(server): add rate limiting and improve handler architecture (#17)

- Add RateLimitDeferrer to handle GitHub API rate limit backoff with exponential retry and jitter
- Create fetcher_test.go with comprehensive test coverage for ConfigFetcher
- Refactor ConfigFetcher constructor for cleaner initialization
- Improve base handler with rate limit context propagation
- Enhance eval context with rate limit aware evaluation
- Add rate limit test suite with mock GitHub client

Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>

Author: denysvitali-kaiko

server/handler/base.go

@@ -44,7 +44,8 @@ type Base struct {
 	AppName string
 	AppID   int64
 
-	Debouncer *StatusDebouncer
+	Debouncer         *StatusDebouncer
+	RateLimitDeferrer *RateLimitDeferrer
 }
 
 // PostCheckRun creates a GitHub check run with consistent logging.
@@ -152,8 +153,8 @@ func (b *Base) newEvalContext(ctx context.Context, installationID int64, loc pul
 }
 
 func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger common.Trigger, loc pull.Locator) error {
+	key := DebounceKey(loc.Owner, loc.Repo, loc.Number)
 	if b.Debouncer != nil {
-		key := DebounceKey(loc.Owner, loc.Repo, loc.Number)
 		trailingFn := func(eventCtx context.Context, accumulated common.Trigger, coalesced int) {
 			tctx, span := StartDebounceTrailingSpan(eventCtx)
 			defer span.End()
@@ -168,7 +169,7 @@ func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger commo
 
 			logger := zerolog.Ctx(tctx)
 			logger.Debug().Msgf("Running trailing evaluation for %s/%s#%d (accumulated trigger: %s, coalesced: %d)", loc.Owner, loc.Repo, loc.Number, accumulated, coalesced)
-			if err := b.doEvaluate(tctx, installationID, accumulated, loc); err != nil {
+			if err := b.evaluateOnce(tctx, installationID, accumulated, loc, key); err != nil {
 				RecordError(span, &err)
 				logger.Error().Err(err).Msgf("Trailing evaluation failed for %s/%s#%d", loc.Owner, loc.Repo, loc.Number)
 			}
@@ -178,7 +179,7 @@ func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger commo
 			return nil
 		}
 	}
-	return b.doEvaluate(ctx, installationID, trigger, loc)
+	return b.evaluateOnce(ctx, installationID, trigger, loc, key)
 }
 
 func (b *Base) doEvaluate(ctx context.Context, installationID int64, trigger common.Trigger, loc pull.Locator) error {
@@ -188,3 +189,28 @@ func (b *Base) doEvaluate(ctx context.Context, installationID int64, trigger com
 	}
 	return evalCtx.Evaluate(ctx, trigger)
 }
+
+func (b *Base) evaluateOnce(ctx context.Context, installationID int64, trigger common.Trigger, loc pull.Locator, key string) error {
+	if b.RateLimitDeferrer != nil && b.RateLimitDeferrer.DeferIfActive(ctx, installationID, key, trigger, func(deferredCtx context.Context, deferredTrigger common.Trigger) {
+		if err := b.evaluateOnce(deferredCtx, installationID, deferredTrigger, loc, key); err != nil {
+			zerolog.Ctx(deferredCtx).Error().Err(err).Msgf("Deferred evaluation failed for %s/%s#%d", loc.Owner, loc.Repo, loc.Number)
+		}
+	}) {
+		zerolog.Ctx(ctx).Warn().Msgf("Deferring evaluation for %s/%s#%d because installation %d is rate limited", loc.Owner, loc.Repo, loc.Number, installationID)
+		return nil
+	}
+
+	if err := b.doEvaluate(ctx, installationID, trigger, loc); err != nil {
+		if resetAt, ok := rateLimitResetTime(err); ok && b.RateLimitDeferrer != nil {
+			b.RateLimitDeferrer.DeferUntil(ctx, installationID, key, trigger, resetAt, func(deferredCtx context.Context, deferredTrigger common.Trigger) {
+				if evalErr := b.evaluateOnce(deferredCtx, installationID, deferredTrigger, loc, key); evalErr != nil {
+					zerolog.Ctx(deferredCtx).Error().Err(evalErr).Msgf("Deferred evaluation failed for %s/%s#%d", loc.Owner, loc.Repo, loc.Number)
+				}
+			})
+			zerolog.Ctx(ctx).Warn().Err(err).Time("github_rate_limit_reset", resetAt).Msgf("Rate limited evaluating %s/%s#%d; deferred retry scheduled", loc.Owner, loc.Repo, loc.Number)
+			return nil
+		}
+		return err
+	}
+	return nil
+}

server/handler/base_test.go

@@ -20,6 +20,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -188,6 +189,80 @@ type recordingTransport struct {
 	configBody     string
 }
 
+func TestEvaluateDefersAndRetriesRateLimitedEvaluation(t *testing.T) {
+	transport := &rateLimitedConfigTransport{
+		resetAt: time.Now().Add(time.Second),
+	}
+	httpClient := &http.Client{Transport: transport}
+
+	client := github.NewClient(httpClient)
+	baseURL, err := url.Parse("http://github.localhost/")
+	require.NoError(t, err)
+	client.BaseURL = baseURL
+
+	v4client := githubv4.NewClient(httpClient)
+
+	deferrer := NewRateLimitDeferrer(0)
+	deferrer.jitter = func() time.Duration { return 0 }
+
+	h := Base{
+		ClientCreator: staticClientCreator{
+			client:   client,
+			v4client: v4client,
+		},
+		ConfigFetcher: NewConfigFetcher(appconfig.NewLoader([]string{".policy.yml"})),
+		BaseConfig: &baseapp.HTTPConfig{
+			PublicURL: "https://policy-bot.example.com",
+		},
+		PullOpts: &PullEvaluationOptions{
+			StatusCheckContext: "policy-bot",
+		},
+		RateLimitDeferrer: deferrer,
+	}
+
+	pr := testPullRequest()
+	err = h.Evaluate(context.Background(), 123, 0, pull.Locator{
+		Owner:  pr.GetBase().GetRepo().GetOwner().GetLogin(),
+		Repo:   pr.GetBase().GetRepo().GetName(),
+		Number: pr.GetNumber(),
+		Value:  pr,
+	})
+	require.NoError(t, err)
+	assert.Equal(t, 1, transport.configRequestCount())
+
+	require.Eventually(t, func() bool {
+		return transport.configRequestCount() >= 2
+	}, 2*time.Second, 25*time.Millisecond)
+}
+
+type rateLimitedConfigTransport struct {
+	mu             sync.Mutex
+	configRequests int
+	resetAt        time.Time
+}
+
+func (rt *rateLimitedConfigTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	rt.mu.Lock()
+	rt.configRequests++
+	count := rt.configRequests
+	rt.mu.Unlock()
+
+	if count == 1 {
+		res := jsonResponse(req, http.StatusForbidden, `{"message":"API rate limit exceeded"}`)
+		res.Header.Set("X-Ratelimit-Limit", "9600")
+		res.Header.Set("X-Ratelimit-Remaining", "0")
+		res.Header.Set("X-Ratelimit-Reset", strconv.FormatInt(rt.resetAt.Unix(), 10))
+		return res, nil
+	}
+	return jsonResponse(req, http.StatusNotFound, `{"message":"Not Found"}`), nil
+}
+
+func (rt *rateLimitedConfigTransport) configRequestCount() int {
+	rt.mu.Lock()
+	defer rt.mu.Unlock()
+	return rt.configRequests
+}
+
 func (rt *recordingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	if req.URL.Path == "/repos/testorg/testrepo/contents/.policy.yml" {
 		time.Sleep(rt.configDelay)

server/handler/debounce.go

@@ -25,7 +25,7 @@ import (
 	"go.opentelemetry.io/otel/trace"
 )
 
-const DefaultDebounceWindow = 5 * time.Second
+const DefaultDebounceWindow = 30 * time.Second
 
 // StatusDebouncer coalesces rapid evaluation requests for the same pull
 // request. When many check runs or status events complete in quick succession

server/handler/eval_context.go

@@ -106,7 +106,7 @@ func parseFetchedConfig(ctx context.Context, fc FetchedConfig, evalOpts *PullEva
 		msg := fmt.Sprintf("Error loading policy from %s", fc.Source)
 		logger.Warn().Err(fc.LoadError).Msg(msg)
 
-		if postStatus != nil {
+		if postStatus != nil && !isTransientClientError(fc.LoadError) {
 			postStatus(ctx, "error", msg)
 		}
 		return nil, errors.Wrapf(fc.LoadError, "failed to load policy: %s: %s", fc.Source, fc.Path)
@@ -319,6 +319,9 @@ func isTransientClientError(err error) bool {
 	if errors.As(err, &ghErr) {
 		return ghErr.Response != nil && ghErr.Response.StatusCode >= 400
 	}
+	if isRateLimitError(err) {
+		return true
+	}
 	var urlErr *url.Error
 	if errors.As(err, &urlErr) {
 		return true

server/handler/eval_context_test.go

@@ -16,7 +16,9 @@ package handler
 
 import (
 	"context"
+	"errors"
 	"testing"
+	"time"
 
 	"github.com/palantir/policy-bot/policy"
 	"github.com/palantir/policy-bot/policy/common"
@@ -243,3 +245,39 @@ func TestEvalContext_EvaluatePolicy_PendingAsFailure_NilConfig(t *testing.T) {
 	assert.Equal(t, "completed", ec.Status.GetStatus(), "server option should be used when policy config is nil")
 	assert.Equal(t, "failure", ec.Status.GetConclusion(), "server option should be used when policy config is nil")
 }
+
+func TestParseFetchedConfigDoesNotPostStatusForTransientLoadError(t *testing.T) {
+	var posted int
+	fc := FetchedConfig{
+		Source:    "testowner/testrepo@main",
+		Path:      ".policy.yml",
+		LoadError: newRateLimitError(time.Now().Add(time.Minute)),
+	}
+
+	_, err := parseFetchedConfig(context.Background(), fc, nil, common.TriggerStatus, func(context.Context, string, string) {
+		posted++
+	})
+
+	require.Error(t, err)
+	assert.Equal(t, 0, posted)
+}
+
+func TestParseFetchedConfigPostsStatusForNonTransientLoadError(t *testing.T) {
+	var posted int
+	fc := FetchedConfig{
+		Source:    "testowner/testrepo@main",
+		Path:      ".policy.yml",
+		LoadError: errors.New("invalid remote reference"),
+	}
+
+	_, err := parseFetchedConfig(context.Background(), fc, nil, common.TriggerStatus, func(context.Context, string, string) {
+		posted++
+	})
+
+	require.Error(t, err)
+	assert.Equal(t, 1, posted)
+}
+
+func TestIsTransientClientErrorRecognizesRateLimits(t *testing.T) {
+	assert.True(t, isTransientClientError(newRateLimitError(time.Now().Add(time.Minute))))
+}

server/handler/fetcher.go

@@ -19,6 +19,7 @@ import (
 	"errors"
 	"net/http"
 	"os"
+	"sync"
 	"time"
 
 	"github.com/google/go-github/v81/github"
@@ -37,10 +38,94 @@ type FetchedConfig struct {
 }
 
 type ConfigFetcher struct {
-	Loader *appconfig.Loader
+	Loader   *appconfig.Loader
+	CacheTTL time.Duration
+	Clock    func() time.Time
+
+	mu       sync.Mutex
+	cache    map[string]configCacheEntry
+	inflight map[string]*configInflight
+}
+
+const DefaultConfigCacheTTL = 30 * time.Second
+
+type configCacheEntry struct {
+	config    FetchedConfig
+	expiresAt time.Time
+}
+
+type configInflight struct {
+	done   chan struct{}
+	config FetchedConfig
+}
+
+func NewConfigFetcher(loader *appconfig.Loader) *ConfigFetcher {
+	return &ConfigFetcher{
+		Loader:   loader,
+		CacheTTL: DefaultConfigCacheTTL,
+	}
 }
 
 func (cf *ConfigFetcher) ConfigForRepositoryBranch(ctx context.Context, client *github.Client, owner, repository, branch string) FetchedConfig {
+	if cf.CacheTTL <= 0 {
+		return cf.loadConfigForRepositoryBranch(ctx, client, owner, repository, branch)
+	}
+
+	key := configCacheKey(owner, repository, branch)
+	now := cf.now()
+
+	cf.mu.Lock()
+	if cf.cache != nil {
+		if entry, ok := cf.cache[key]; ok {
+			if now.Before(entry.expiresAt) {
+				cf.mu.Unlock()
+				return cloneFetchedConfig(entry.config)
+			}
+			delete(cf.cache, key)
+		}
+	}
+	if cf.inflight != nil {
+		if in := cf.inflight[key]; in != nil {
+			cf.mu.Unlock()
+			select {
+			case <-ctx.Done():
+				return FetchedConfig{
+					Source:    owner + "/" + repository + "@" + branch,
+					LoadError: ctx.Err(),
+				}
+			case <-in.done:
+				return cloneFetchedConfig(in.config)
+			}
+		}
+	}
+	if cf.inflight == nil {
+		cf.inflight = make(map[string]*configInflight)
+	}
+	in := &configInflight{done: make(chan struct{})}
+	cf.inflight[key] = in
+	cf.mu.Unlock()
+
+	fc := cf.loadConfigForRepositoryBranch(ctx, client, owner, repository, branch)
+
+	cf.mu.Lock()
+	in.config = cloneFetchedConfig(fc)
+	delete(cf.inflight, key)
+	if shouldCacheFetchedConfig(fc) {
+		if cf.cache == nil {
+			cf.cache = make(map[string]configCacheEntry)
+		}
+		cf.cache[key] = configCacheEntry{
+			config:    cloneFetchedConfig(fc),
+			expiresAt: cf.now().Add(cf.CacheTTL),
+		}
+	}
+	close(in.done)
+	cf.mu.Unlock()
+
+	return fc
+}
+
+func (cf *ConfigFetcher) loadConfigForRepositoryBranch(ctx context.Context, client *github.Client, owner, repository, branch string) FetchedConfig {
 	retries := 0
 	delay := 1 * time.Second
 	for {
@@ -86,6 +171,39 @@ func (cf *ConfigFetcher) ConfigForRepositoryBranch(ctx context.Context, client *
 	}
 }
 
+func (cf *ConfigFetcher) now() time.Time {
+	if cf.Clock != nil {
+		return cf.Clock()
+	}
+	return time.Now()
+}
+
+func configCacheKey(owner, repository, branch string) string {
+	return owner + "/" + repository + "@" + branch
+}
+
+func shouldCacheFetchedConfig(fc FetchedConfig) bool {
+	return fc.LoadError == nil && fc.ParseError == nil
+}
+
+func cloneFetchedConfig(fc FetchedConfig) FetchedConfig {
+	clone := fc
+	if fc.Config == nil {
+		return clone
+	}
+
+	b, err := yaml.Marshal(fc.Config)
+	if err != nil {
+		return clone
+	}
+	var pc policy.Config
+	if err := yaml.UnmarshalStrict(b, &pc); err != nil {
+		return clone
+	}
+	clone.Config = &pc
+	return clone
+}
+
 func isServerError(err error) bool {
 	var ghErr *github.ErrorResponse
 	if errors.As(err, &ghErr) {