Skip to content

A-1320 retry cache commands #3963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
buildkate merged 1 commit into from
May 28, 2026
+242 −119
Merged

A-1320 retry cache commands #3963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 65 additions & 60 deletions api/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,121 +121,124 @@ type CacheEntryCommitResp struct {
}

// CacheRegistry retrieves information about a cache registry.
func (c *Client) CacheRegistry(ctx context.Context, registry string) (CacheRegistryResp, error) {
func (c *Client) CacheRegistry(ctx context.Context, registry string) (CacheRegistryResp, *Response, error) {
ctx, span := cacheTracer.Start(ctx, "Client.CacheRegistry")
defer span.End()

var resp CacheRegistryResp
var cacheResp CacheRegistryResp

req, err := c.newRequest(ctx, http.MethodGet, cachePath("/cache_registries/%s", registry), nil)
if err != nil {
return resp, cacheSpanErr(span, "failed to create request: %w", err)
return cacheResp, nil, cacheSpanErr(span, "failed to create request: %w", err)
}

res, err := c.cacheDo(req, &resp)
apiResp, err := c.cacheDo(req, &cacheResp)
if err != nil {
return resp, cacheSpanErr(span, "%w", err)
return cacheResp, apiResp, cacheSpanErr(span, "%w", err)
}
if res.StatusCode != http.StatusOK {
return resp, cacheSpanErr(span, "failed to get cache registry: %s", res.Status)
if apiResp.StatusCode != http.StatusOK {
return cacheResp, apiResp, cacheSpanErr(span, "failed to get cache registry: %s", apiResp.Status)
}
return resp, nil
return cacheResp, apiResp, nil
}

// CacheEntryPeekExists checks whether a cache entry exists.
// Returns (resp, true, nil) on hit, (resp, false, nil) on miss (HTTP 404 with
// CacheEntryNotFound), or (resp, false, err) on any other failure.
func (c *Client) CacheEntryPeekExists(ctx context.Context, registry string, peek CacheEntryPeekReq) (CacheEntryPeekResp, bool, error) {
// Returns (resp, true, _, nil) on hit, (resp, false, _, nil) on miss (HTTP 404
// with CacheEntryNotFound), or (resp, false, _, err) on any other failure.
func (c *Client) CacheEntryPeekExists(ctx context.Context, registry string, peek CacheEntryPeekReq) (CacheEntryPeekResp, bool, *Response, error) {
ctx, span := cacheTracer.Start(ctx, "Client.CacheEntryPeekExists")
defer span.End()

var resp CacheEntryPeekResp
var cacheResp CacheEntryPeekResp

path, err := cacheQueryPath("/cache_registries/%s/peek", registry, peek)
if err != nil {
return resp, false, cacheSpanErr(span, "%w", err)
return cacheResp, false, nil, cacheSpanErr(span, "%w", err)
}

req, err := c.newRequest(ctx, http.MethodGet, path, nil)
if err != nil {
return resp, false, cacheSpanErr(span, "failed to create request: %w", err)
return cacheResp, false, nil, cacheSpanErr(span, "failed to create request: %w", err)
}

res, err := c.cacheDo(req, &resp)
apiResp, err := c.cacheDo(req, &cacheResp)
if err != nil {
return resp, false, cacheSpanErr(span, "%w", err)
return cacheResp, false, apiResp, cacheSpanErr(span, "%w", err)
}
return interpretCacheResponse(span, res, resp)
cacheResp, exists, err := interpretCacheResponse(span, apiResp, cacheResp)
return cacheResp, exists, apiResp, err
}

// CacheEntryCreate creates a new cache entry and returns upload instructions.
func (c *Client) CacheEntryCreate(ctx context.Context, registry string, create CacheEntryCreateReq) (CacheEntryCreateResp, error) {
func (c *Client) CacheEntryCreate(ctx context.Context, registry string, create CacheEntryCreateReq) (CacheEntryCreateResp, *Response, error) {
ctx, span := cacheTracer.Start(ctx, "Client.CacheEntryCreate")
defer span.End()

var resp CacheEntryCreateResp
var cacheResp CacheEntryCreateResp

req, err := c.newRequest(ctx, http.MethodPut, cachePath("/cache_registries/%s/store", registry), &create)
if err != nil {
return resp, cacheSpanErr(span, "failed to create request: %w", err)
return cacheResp, nil, cacheSpanErr(span, "failed to create request: %w", err)
}

res, err := c.cacheDo(req, &resp)
apiResp, err := c.cacheDo(req, &cacheResp)
if err != nil {
return resp, cacheSpanErr(span, "%w", err)
return cacheResp, apiResp, cacheSpanErr(span, "%w", err)
}
if res.StatusCode != http.StatusOK {
return resp, cacheSpanErr(span, "failed to save: %s", res.Status)
if apiResp.StatusCode != http.StatusOK {
return cacheResp, apiResp, cacheSpanErr(span, "failed to save: %s", apiResp.Status)
}
return resp, nil
return cacheResp, apiResp, nil
}

// CacheEntryCommit marks a previously created cache entry as committed.
func (c *Client) CacheEntryCommit(ctx context.Context, registry string, commit CacheEntryCommitReq) (CacheEntryCommitResp, error) {
func (c *Client) CacheEntryCommit(ctx context.Context, registry string, commit CacheEntryCommitReq) (CacheEntryCommitResp, *Response, error) {
ctx, span := cacheTracer.Start(ctx, "Client.CacheEntryCommit")
defer span.End()

var resp CacheEntryCommitResp
var cacheResp CacheEntryCommitResp

req, err := c.newRequest(ctx, http.MethodPut, cachePath("/cache_registries/%s/commit", registry), &commit)
if err != nil {
return resp, cacheSpanErr(span, "failed to create request: %w", err)
return cacheResp, nil, cacheSpanErr(span, "failed to create request: %w", err)
}

res, err := c.cacheDo(req, &resp)
apiResp, err := c.cacheDo(req, &cacheResp)
if err != nil {
return resp, cacheSpanErr(span, "%w", err)
return cacheResp, apiResp, cacheSpanErr(span, "%w", err)
}
if res.StatusCode != http.StatusOK {
return resp, cacheSpanErr(span, "failed to commit: %s", res.Status)
if apiResp.StatusCode != http.StatusOK {
return cacheResp, apiResp, cacheSpanErr(span, "failed to commit: %s", apiResp.Status)
}
return resp, nil
return cacheResp, apiResp, nil
}

// CacheEntryRetrieve retrieves download instructions for a cache entry.
// Returns (resp, true, nil) on hit (possibly via a fallback key), (resp, false,
// nil) on miss, or (resp, false, err) on any other failure.
func (c *Client) CacheEntryRetrieve(ctx context.Context, registry string, retrieve CacheEntryRetrieveReq) (CacheEntryRetrieveResp, bool, error) {
// Returns (resp, true, _, nil) on hit (possibly via a fallback key),
// (resp, false, _, nil) on miss, or (resp, false, _, err) on any other failure.
func (c *Client) CacheEntryRetrieve(ctx context.Context, registry string, retrieve CacheEntryRetrieveReq) (CacheEntryRetrieveResp, bool, *Response, error) {
ctx, span := cacheTracer.Start(ctx, "Client.CacheEntryRetrieve")
defer span.End()

var resp CacheEntryRetrieveResp
var cacheResp CacheEntryRetrieveResp

path, err := cacheQueryPath("/cache_registries/%s/retrieve", registry, retrieve)
if err != nil {
return resp, false, cacheSpanErr(span, "%w", err)
return cacheResp, false, nil, cacheSpanErr(span, "%w", err)
}

req, err := c.newRequest(ctx, http.MethodGet, path, nil)
if err != nil {
return resp, false, cacheSpanErr(span, "failed to create request: %w", err)
return cacheResp, false, nil, cacheSpanErr(span, "failed to create request: %w", err)
}

res, err := c.cacheDo(req, &resp)
apiResp, err := c.cacheDo(req, &cacheResp)
if err != nil {
return resp, false, cacheSpanErr(span, "%w", err)
return cacheResp, false, apiResp, cacheSpanErr(span, "%w", err)
}
return interpretCacheResponse(span, res, resp)

cacheResp, exists, err := interpretCacheResponse(span, apiResp, cacheResp)
return cacheResp, exists, apiResp, err
}

// cachePath formats a cache API path with URL-safe escaping for path components.
Expand All @@ -260,7 +263,7 @@ func cacheQueryPath(format, registry string, params any) (string, error) {
// and decodes a JSON body into resp. Unlike Client.doRequest, it does not
// treat non-2xx as an error — cache responses use 404 + a message to signal a
// miss, and callers want to inspect the status themselves.
func (c *Client) cacheDo(req *http.Request, resp any) (*http.Response, error) {
func (c *Client) cacheDo(req *http.Request, resp any) (*Response, error) {
httpResp, err := agenthttp.Do(c.logger, c.client, req,
agenthttp.WithDebugHTTP(c.conf.DebugHTTP),
agenthttp.WithTraceHTTP(c.conf.TraceHTTP),
Expand All @@ -271,29 +274,31 @@ func (c *Client) cacheDo(req *http.Request, resp any) (*http.Response, error) {
defer httpResp.Body.Close() //nolint:errcheck
defer io.Copy(io.Discard, httpResp.Body) //nolint:errcheck

apiResp := newResponse(httpResp)

if httpResp.StatusCode >= 500 {
return httpResp, fmt.Errorf("request failed with status: %s", httpResp.Status)
return apiResp, fmt.Errorf("request failed with status: %s", httpResp.Status)
}
if httpResp.Body == http.NoBody {
return httpResp, nil
return apiResp, nil
}

contentType := httpResp.Header.Get("Content-Type")
if !isJSONContent(contentType) {
return httpResp, fmt.Errorf("unexpected content type: %s", contentType)
return apiResp, fmt.Errorf("unexpected content type: %s", contentType)
}

body, err := io.ReadAll(httpResp.Body)
if err != nil {
return httpResp, fmt.Errorf("failed to read response body: %w", err)
return apiResp, fmt.Errorf("failed to read response body: %w", err)
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't mark mid-body EOFs non-retryable

In the new cache retry paths, this returns a non-nil response even when reading the response body fails. If the cache API sends headers (for example a 200) and then the connection drops or truncates the body, the error can wrap EOF/connection reset, but BreakOnNonRetryable checks the response status first; because 200 is not 429/5xx it calls Break() and the retrier gives up after the first attempt. That skips the intended retry for transient EOF/connection errors in this scenario, so the body transport error should be classified without the successful response status blocking IsRetryableError.

Useful? React with 👍 / 👎.

}
if len(body) == 0 {
return httpResp, nil
return apiResp, nil
}
if err := json.Unmarshal(body, resp); err != nil {
return httpResp, fmt.Errorf("failed to decode response body: %w", err)
return apiResp, fmt.Errorf("failed to decode response body: %w", err)
}
return httpResp, nil
return apiResp, nil
}

// cacheMessage is the subset of cache responses needed to classify a 404.
Expand All @@ -306,23 +311,23 @@ func (r CacheEntryRetrieveResp) cacheMessage() string { return r.Message }

// interpretCacheResponse maps the dual "200 = hit, 404 + message = miss"
// convention into the (resp, exists, err) return shape used by peek/retrieve.
func interpretCacheResponse[T cacheMessage](span oteltrace.Span, res *http.Response, resp T) (T, bool, error) {
if res.StatusCode == http.StatusOK {
return resp, true, nil
func interpretCacheResponse[T cacheMessage](span oteltrace.Span, apiResp *Response, cacheResp T) (T, bool, error) {
if apiResp.StatusCode == http.StatusOK {
return cacheResp, true, nil
}
switch res.StatusCode {
switch apiResp.StatusCode {
case http.StatusNotFound:
switch resp.cacheMessage() {
switch cacheResp.cacheMessage() {
case CacheEntryNotFound:
return resp, false, nil
return cacheResp, false, nil
case CacheRegistryNotFound:
return resp, false, cacheSpanErr(span, "cache registry not found: %s", res.Status)
return cacheResp, false, cacheSpanErr(span, "cache registry not found: %s", apiResp.Status)
}
return resp, false, cacheSpanErr(span, "not found: %s", res.Status)
return cacheResp, false, cacheSpanErr(span, "not found: %s", apiResp.Status)
case http.StatusBadRequest:
return resp, false, cacheSpanErr(span, "bad request: %s", res.Status)
return cacheResp, false, cacheSpanErr(span, "bad request: %s", apiResp.Status)
default:
return resp, false, cacheSpanErr(span, "request failed with status: %s", res.Status)
return cacheResp, false, cacheSpanErr(span, "request failed with status: %s", apiResp.Status)
}
}

Expand Down
20 changes: 10 additions & 10 deletions api/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestCacheEntryPeekExists_Success(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, exists, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
resp, exists, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
Key: "test-key",
Branch: "main",
})
Expand All @@ -66,7 +66,7 @@ func TestCacheEntryPeekExists_NotFound(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, exists, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
resp, exists, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
Key: "nonexistent-key",
Branch: "main",
})
Expand All @@ -91,7 +91,7 @@ func TestCacheEntryPeekExists_WrongContentType(t *testing.T) {

client := newTestCacheClient(t, server.URL)

_, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
_, _, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
Key: "test-key",
Branch: "main",
})
Expand All @@ -110,7 +110,7 @@ func TestCacheEntryPeekExists_CacheRegistryNotFound(t *testing.T) {

client := newTestCacheClient(t, server.URL)

_, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
_, _, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
Key: "test-key",
Branch: "main",
})
Expand All @@ -129,7 +129,7 @@ func TestCacheEntryPeekExists_ContentTypeWithCharset(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, exists, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
resp, exists, _, err := client.CacheEntryPeekExists(context.Background(), "test-slug", api.CacheEntryPeekReq{
Key: "test-key",
Branch: "main",
})
Expand Down Expand Up @@ -170,7 +170,7 @@ func TestCacheEntryCreate_Success(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, err := client.CacheEntryCreate(context.Background(), "test-slug", api.CacheEntryCreateReq{
resp, _, err := client.CacheEntryCreate(context.Background(), "test-slug", api.CacheEntryCreateReq{
Key: "test-key",
FallbackKeys: []string{"fallback-1", "fallback-2"},
Compression: "gzip",
Expand Down Expand Up @@ -217,7 +217,7 @@ func TestCacheEntryRetrieve_Success(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, found, err := client.CacheEntryRetrieve(context.Background(), "test-slug", api.CacheEntryRetrieveReq{
resp, found, _, err := client.CacheEntryRetrieve(context.Background(), "test-slug", api.CacheEntryRetrieveReq{
Key: "test-key",
Branch: "main",
FallbackKeys: "fallback-1,fallback-2",
Expand Down Expand Up @@ -246,7 +246,7 @@ func TestCacheEntryRetrieve_NotFound(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, found, err := client.CacheEntryRetrieve(context.Background(), "test-slug", api.CacheEntryRetrieveReq{
resp, found, _, err := client.CacheEntryRetrieve(context.Background(), "test-slug", api.CacheEntryRetrieveReq{
Key: "nonexistent-key",
Branch: "main",
})
Expand Down Expand Up @@ -282,7 +282,7 @@ func TestCacheEntryCommit_Success(t *testing.T) {

client := newTestCacheClient(t, server.URL)

resp, err := client.CacheEntryCommit(context.Background(), "test-slug", api.CacheEntryCommitReq{
resp, _, err := client.CacheEntryCommit(context.Background(), "test-slug", api.CacheEntryCommitReq{
UploadID: "upload-123",
})
if err != nil {
Expand All @@ -303,7 +303,7 @@ func TestCacheEntryCommit_Failure(t *testing.T) {

client := newTestCacheClient(t, server.URL)

_, err := client.CacheEntryCommit(context.Background(), "test-slug", api.CacheEntryCommitReq{
_, _, err := client.CacheEntryCommit(context.Background(), "test-slug", api.CacheEntryCommitReq{
UploadID: "invalid-upload-id",
})
if err == nil {
Expand Down
10 changes: 5 additions & 5 deletions internal/cache/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ import (
// server returning canned responses — this repo prefers real HTTP servers
// over hand-rolled mocks.
type cacheAPI interface {
CacheRegistry(ctx context.Context, registry string) (api.CacheRegistryResp, error)
CacheEntryPeekExists(ctx context.Context, registry string, req api.CacheEntryPeekReq) (api.CacheEntryPeekResp, bool, error)
CacheEntryCreate(ctx context.Context, registry string, req api.CacheEntryCreateReq) (api.CacheEntryCreateResp, error)
CacheEntryCommit(ctx context.Context, registry string, req api.CacheEntryCommitReq) (api.CacheEntryCommitResp, error)
CacheEntryRetrieve(ctx context.Context, registry string, req api.CacheEntryRetrieveReq) (api.CacheEntryRetrieveResp, bool, error)
CacheRegistry(ctx context.Context, registry string) (api.CacheRegistryResp, *api.Response, error)
CacheEntryPeekExists(ctx context.Context, registry string, req api.CacheEntryPeekReq) (api.CacheEntryPeekResp, bool, *api.Response, error)
CacheEntryCreate(ctx context.Context, registry string, req api.CacheEntryCreateReq) (api.CacheEntryCreateResp, *api.Response, error)
CacheEntryCommit(ctx context.Context, registry string, req api.CacheEntryCommitReq) (api.CacheEntryCommitResp, *api.Response, error)
CacheEntryRetrieve(ctx context.Context, registry string, req api.CacheEntryRetrieveReq) (api.CacheEntryRetrieveResp, bool, *api.Response, error)
}

// Sentinel errors for common scenarios.
Expand Down
Loading