Evaluator: enable per-tenant HTTP caching for GitHub API client by edigaryev · Pull Request #953 · cirruslabs/cirrus-cli

edigaryev · 2025-09-30T14:35:00Z

To reduce GitHub API rate limit usage for Starlark scripts.

fkorotkov · 2025-09-30T14:37:52Z

pkg/larker/fs/github/github.go

+	httpClient := httpcache.NewClient("memcache://")
+
+	// GitHub has a 10-second timeout for API requests
+	httpClient.Timeout = 11 * time.Second


Is it a TTL for caching or just request timeout?

Request timeout, this is copied from the previous code.

fkorotkov · 2025-09-30T14:43:26Z

pkg/larker/fs/github/github.go

 }

 func (gh *GitHub) client() *github.Client {
+	httpClient := httpcache.NewClient("memcache://")


I think this should be a singleton shared between these instances. Otherwise we'll only cache things within a single starlark evaluation.

Maybe pass fs_cachable in the proto request.

I think this should be a singleton shared between these instances. Otherwise we'll only cache things within a single starlark evaluation.

Fixed in 267599f.

Maybe pass fs_cachable in the proto request.

Can you elaborate?

Was thinking if it's safe to share the cache between repositories for a GH FS or should be have a cache of FSes per repo.

I think the safest (and good enough) way to start with is to have a separate cache, HTTP client and GitHub client instance per EvaluateConfig() and EvaluateFunction().

The current implementation is almost like that, except that it instantiates a new cache, HTTP client and GitHub client instance on module loading (e.g. load("github.com/cirrus-modules/golang", "task", "container")).

It feels like we should avoid doing this if currentFS is already *github.GitHub here:

cirrus-cli/pkg/larker/resolver/module_fs.go

Lines 107 to 120 in fdfa078

func findLocatorFS(

ctx context.Context,

currentFS fs.FileSystem,

env map[string]string,

location interface{},

) (fs.FileSystem, string, error) {

switch typedLocation := location.(type) {

case gitHubLocation:

token := env["CIRRUS_REPO_CLONE_TOKEN"]

ghFS, err := github.New(typedLocation.Owner, typedLocation.Name, typedLocation.Revision, token)

if err != nil {

return nil, "", err

}

Actually, perhaps a separate *github.Github is totally fine, we just need to have a shared *http.Client per EvaluateConfig() and EvaluateFunction() invocation.

Actually, perhaps a separate *github.Github is totally fine, we just need to have a shared *http.Client per EvaluateConfig() and EvaluateFunction() invocation.

Implemented in 792cb3a.

fkorotkov · 2025-09-30T14:48:18Z

pkg/larker/fs/github/github.go


-var defaultGitHubClient = github.NewClient(&http.Client{
-	Transport: &http.Transport{
+var httpClient = httpcache.NewClient("memcache://", httpcache.WithUpstream(


Does it evict things ever?

Good catch. I think we need re-initialize the caching HTTP client per-evaluation 🤔

Fixed in af92585.

fkorotkov · 2025-10-02T12:10:01Z

internal/evaluator/evaluator.go

 }
+
+func cachingHTTPClient() *http.Client {
+	httpClient := httpcache.NewClient("memcache://", httpcache.WithUpstream(


Does it ever invalidate entries?

It does:

NewClient(dsn, ...) calls NewTransport(dsn, ...)

NewTransport(dsn, ...) calls store.Open(dsn)

store.Open(dsn) calls registry.Default().OpenConn(dsn)

registry.Default().OpenConn(dsn) calls memcache.Open(), which creates a new in-memory map

I just don't see how it caches between invocations of EvaluateConfig. To me it seems it only caches within a single invocation of EvaluateConfig which is still good but probably not effective at all.

I just don't see how it caches between invocations of EvaluateConfig.

It does not for security reasons. Otherwise that would be considered a shared cache, which github.com/bartventer/httpcache is not:

Note: This package is intended for use as a private (client-side) cache. It is not a shared or proxy cache. It is designed to be used with an HTTP client to cache responses from origin servers, improving performance and reducing load on those servers.

Also for shared caches, storing responses to authenticated requests is not allowed as per RFC 9111, §3:

A cache MUST NOT store a response to a request unless:

[...]

if the cache is shared: the Authorization header field is not present in the request (see Section 11.6.2 of [HTTP]) or a response directive is present that explicitly allows shared caching (see Section 3.5); and

We do some exceptions to this in Chacha, but in case of Chacha we control much more variables (e.g. we specifically list HTTP targets for exclusion), compared to all of the GitHub API.

OK. Then I'm not going crazy. Exactly why I initially proposed to have a cache per GitHub Repo.

Larker: enable HTTP caching for GitHub API client

e6ba27e

edigaryev requested a review from fkorotkov as a code owner September 30, 2025 14:35

Bring back #689 tuning

529ce35

fkorotkov reviewed Sep 30, 2025

View reviewed changes

edigaryev force-pushed the larker-github-client-caching branch from 017de46 to 529ce35 Compare September 30, 2025 14:43

Initialize HTTPClient once

267599f

edigaryev requested a review from fkorotkov September 30, 2025 14:46

Only use init() for tuning the HTTPClient after it's initialized

07175f2

fkorotkov reviewed Sep 30, 2025

View reviewed changes

edigaryev added 2 commits September 30, 2025 16:52

Per-instance HTTPClient and GitHub client

af92585

Share caching *http.Client for multiple github.New() invocations

792cb3a

edigaryev requested a review from fkorotkov September 30, 2025 15:13

fkorotkov reviewed Oct 2, 2025

View reviewed changes

edigaryev requested a review from fkorotkov October 2, 2025 12:23

edigaryev added 4 commits October 2, 2025 21:13

Per-tenant shared HTTP cache

593de15

Use Get() methods to prevent NPE

1b09de0

Introduce TestGitHubHTTPCache

6343e2a

TestGitHubHTTPCache: test with two separate tenants

5a88a67

edigaryev changed the title ~~Larker: enable HTTP caching for GitHub API client~~ Larker: enable per-tenant HTTP caching for GitHub API client Oct 2, 2025

edigaryev changed the title ~~Larker: enable per-tenant HTTP caching for GitHub API client~~ Evaluator: enable per-tenant HTTP caching for GitHub API client Oct 2, 2025

fkorotkov approved these changes Oct 6, 2025

View reviewed changes

edigaryev added 2 commits October 6, 2025 18:07

Merge branch 'main' into larker-github-client-caching

4dc72f1

$ go mod tidy

bbb0e4b

edigaryev merged commit 3af5e7d into main Oct 6, 2025
9 of 11 checks passed

edigaryev deleted the larker-github-client-caching branch October 6, 2025 16:28

	func findLocatorFS(
	ctx context.Context,
	currentFS fs.FileSystem,
	env map[string]string,
	location interface{},
	) (fs.FileSystem, string, error) {
	switch typedLocation := location.(type) {
	case gitHubLocation:
	token := env["CIRRUS_REPO_CLONE_TOKEN"]

	ghFS, err := github.New(typedLocation.Owner, typedLocation.Name, typedLocation.Revision, token)
	if err != nil {
	return nil, "", err
	}

Conversation

edigaryev commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edigaryev Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edigaryev Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edigaryev Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

edigaryev commented Sep 30, 2025 •

edited

Loading

edigaryev Sep 30, 2025 •

edited

Loading

edigaryev Oct 2, 2025 •

edited

Loading

edigaryev Oct 2, 2025 •

edited

Loading