fix(preview): retry transient sprites errors (#2116)
This commit is contained in:
Carlos Florêncio
@@ -5,6 +5,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net"
|
||||
"os"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -13,10 +14,11 @@ import (
|
||||
)
|
||||
|
||||
const (
|
||||
spriteUploadTimeout = 10 * time.Minute // bundles can be large
|
||||
spriteStepTimeout = 2 * time.Minute
|
||||
spriteUploadRetries = 3
|
||||
spriteBackoffInit = 700 * time.Millisecond
|
||||
spriteUploadTimeout = 10 * time.Minute // bundles can be large
|
||||
spriteStepTimeout = 2 * time.Minute
|
||||
spriteControlRetries = 3
|
||||
spriteUploadRetries = 3
|
||||
spriteBackoffInit = 700 * time.Millisecond
|
||||
)
|
||||
|
||||
func newSpriteClient(token string) *sprites.Client {
|
||||
@@ -26,27 +28,81 @@ func newSpriteClient(token string) *sprites.Client {
|
||||
// getOrCreateSprite returns an existing sprite or creates a new one.
|
||||
// Cold/sleeping sprites wake automatically when commands are issued.
|
||||
func getOrCreateSprite(ctx context.Context, client *sprites.Client, name string) (*sprites.Sprite, error) {
|
||||
stepCtx, cancel := context.WithTimeout(ctx, spriteStepTimeout)
|
||||
defer cancel()
|
||||
var lastErr error
|
||||
for attempt := 1; attempt <= spriteControlRetries; attempt++ {
|
||||
stepCtx, cancel := context.WithTimeout(ctx, spriteStepTimeout)
|
||||
sprite, err := client.GetSprite(stepCtx, name)
|
||||
cancel()
|
||||
if err == nil {
|
||||
return sprite, nil
|
||||
}
|
||||
|
||||
sprite, err := client.GetSprite(stepCtx, name)
|
||||
if err == nil {
|
||||
return sprite, nil
|
||||
if isSpriteNotFound(err) {
|
||||
createCtx, createCancel := context.WithTimeout(ctx, spriteStepTimeout)
|
||||
sprite, err = client.CreateSprite(createCtx, name, nil)
|
||||
createCancel()
|
||||
if err == nil {
|
||||
return sprite, nil
|
||||
}
|
||||
lastErr = fmt.Errorf("create sprite: %w", err)
|
||||
} else {
|
||||
lastErr = fmt.Errorf("get sprite: %w", err)
|
||||
}
|
||||
|
||||
if !isTransientSpriteError(lastErr) || attempt == spriteControlRetries {
|
||||
return nil, lastErr
|
||||
}
|
||||
if err := waitForSpriteRetry(ctx, "get/create sprite", attempt, lastErr); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
return nil, lastErr
|
||||
}
|
||||
|
||||
func isSpriteNotFound(err error) bool {
|
||||
// The SDK returns a plain fmt.Errorf("sprite not found: %s") for 404s —
|
||||
// no typed error is available, so we check the message string.
|
||||
if !strings.Contains(err.Error(), "not found") {
|
||||
return nil, fmt.Errorf("get sprite: %w", err)
|
||||
return strings.Contains(err.Error(), "not found")
|
||||
}
|
||||
|
||||
func isTransientSpriteError(err error) bool {
|
||||
if errors.Is(err, context.DeadlineExceeded) {
|
||||
return true
|
||||
}
|
||||
|
||||
createCtx, createCancel := context.WithTimeout(ctx, spriteStepTimeout)
|
||||
defer createCancel()
|
||||
|
||||
sprite, err = client.CreateSprite(createCtx, name, nil)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("create sprite: %w", err)
|
||||
var networkErr net.Error
|
||||
if errors.As(err, &networkErr) && networkErr.Timeout() {
|
||||
return true
|
||||
}
|
||||
// Temporary is deprecated on net.Error, but older transports still expose
|
||||
// it. Keep compatibility through a local interface without calling the
|
||||
// deprecated method on net.Error directly.
|
||||
var temporaryErr interface{ Temporary() bool }
|
||||
if errors.As(err, &temporaryErr) && temporaryErr.Temporary() {
|
||||
return true
|
||||
}
|
||||
|
||||
var apiErr *sprites.APIError
|
||||
if errors.As(err, &apiErr) {
|
||||
return apiErr.StatusCode == 429 || apiErr.StatusCode >= 500
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func waitForSpriteRetry(ctx context.Context, operation string, attempt int, err error) error {
|
||||
delay := spriteBackoffInit * time.Duration(1<<(attempt-1))
|
||||
var apiErr *sprites.APIError
|
||||
if errors.As(err, &apiErr) && apiErr.GetRetryAfterSeconds() > 0 {
|
||||
delay = time.Duration(apiErr.GetRetryAfterSeconds()) * time.Second
|
||||
}
|
||||
|
||||
fmt.Fprintf(os.Stderr, " %s attempt %d failed (%v), retrying in %v...\n", operation, attempt, err, delay)
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
return ctx.Err()
|
||||
case <-time.After(delay):
|
||||
return nil
|
||||
}
|
||||
return sprite, nil
|
||||
}
|
||||
|
||||
// uploadBundle uploads the bundle tarball to the sprite via the Filesystem API.
|
||||
|
||||
@@ -1,10 +1,141 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
sprites "github.com/superfly/sprites-go"
|
||||
)
|
||||
|
||||
type testNetworkError struct {
|
||||
temporary bool
|
||||
}
|
||||
|
||||
func (e testNetworkError) Error() string { return "test network error" }
|
||||
func (e testNetworkError) Timeout() bool { return false }
|
||||
func (e testNetworkError) Temporary() bool { return e.temporary }
|
||||
|
||||
func TestIsTransientSpriteErrorClassifiesTemporaryNetworkErrors(t *testing.T) {
|
||||
if !isTransientSpriteError(fmt.Errorf("lookup: %w", testNetworkError{temporary: true})) {
|
||||
t.Fatal("temporary network error should be retryable")
|
||||
}
|
||||
if isTransientSpriteError(testNetworkError{}) {
|
||||
t.Fatal("permanent network error should not be retryable")
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateSpriteRetriesTransientGet(t *testing.T) {
|
||||
getCalls := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
getCalls++
|
||||
if getCalls == 1 {
|
||||
w.WriteHeader(http.StatusServiceUnavailable)
|
||||
return
|
||||
}
|
||||
_, _ = w.Write([]byte(`{"name":"kandev-pr-2115","status":"created"}`))
|
||||
}))
|
||||
t.Cleanup(server.Close)
|
||||
|
||||
client := sprites.New("token", sprites.WithBaseURL(server.URL))
|
||||
t.Cleanup(func() { _ = client.Close() })
|
||||
|
||||
sprite, err := getOrCreateSprite(t.Context(), client, "kandev-pr-2115")
|
||||
if err != nil {
|
||||
t.Fatalf("getOrCreateSprite() error = %v", err)
|
||||
}
|
||||
if sprite.Name() != "kandev-pr-2115" {
|
||||
t.Errorf("sprite.Name() = %q", sprite.Name())
|
||||
}
|
||||
if getCalls != 2 {
|
||||
t.Errorf("get calls = %d, want 2", getCalls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateSpriteReconcilesTransientCreateFailure(t *testing.T) {
|
||||
getCalls := 0
|
||||
createCalls := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
switch r.Method {
|
||||
case http.MethodGet:
|
||||
getCalls++
|
||||
if getCalls == 1 {
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
return
|
||||
}
|
||||
_, _ = w.Write([]byte(`{"name":"kandev-pr-2115","status":"created"}`))
|
||||
case http.MethodPost:
|
||||
createCalls++
|
||||
w.WriteHeader(http.StatusServiceUnavailable)
|
||||
default:
|
||||
t.Errorf("unexpected %s request", r.Method)
|
||||
}
|
||||
}))
|
||||
t.Cleanup(server.Close)
|
||||
|
||||
client := sprites.New("token", sprites.WithBaseURL(server.URL))
|
||||
t.Cleanup(func() { _ = client.Close() })
|
||||
|
||||
sprite, err := getOrCreateSprite(t.Context(), client, "kandev-pr-2115")
|
||||
if err != nil {
|
||||
t.Fatalf("getOrCreateSprite() error = %v", err)
|
||||
}
|
||||
if sprite.Name() != "kandev-pr-2115" {
|
||||
t.Errorf("sprite.Name() = %q", sprite.Name())
|
||||
}
|
||||
if createCalls != 1 {
|
||||
t.Errorf("create calls = %d, want 1", createCalls)
|
||||
}
|
||||
if getCalls != 2 {
|
||||
t.Errorf("get calls = %d, want 2", getCalls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateSpriteDoesNotRetryPermanentErrors(t *testing.T) {
|
||||
getCalls := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
getCalls++
|
||||
w.WriteHeader(http.StatusUnauthorized)
|
||||
}))
|
||||
t.Cleanup(server.Close)
|
||||
|
||||
client := sprites.New("token", sprites.WithBaseURL(server.URL))
|
||||
t.Cleanup(func() { _ = client.Close() })
|
||||
|
||||
_, err := getOrCreateSprite(t.Context(), client, "kandev-pr-2115")
|
||||
if err == nil {
|
||||
t.Fatal("getOrCreateSprite() error = nil, want permanent error")
|
||||
}
|
||||
if getCalls != 1 {
|
||||
t.Errorf("get calls = %d, want 1", getCalls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetOrCreateSpriteReturnsFinalTransientErrorAfterRetryBudget(t *testing.T) {
|
||||
getCalls := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
getCalls++
|
||||
w.WriteHeader(http.StatusServiceUnavailable)
|
||||
}))
|
||||
t.Cleanup(server.Close)
|
||||
|
||||
client := sprites.New("token", sprites.WithBaseURL(server.URL))
|
||||
t.Cleanup(func() { _ = client.Close() })
|
||||
|
||||
_, err := getOrCreateSprite(t.Context(), client, "kandev-pr-2115")
|
||||
if err == nil {
|
||||
t.Fatal("getOrCreateSprite() error = nil, want transient error")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "get sprite") {
|
||||
t.Errorf("getOrCreateSprite() error = %v, want get sprite error", err)
|
||||
}
|
||||
if getCalls != spriteControlRetries {
|
||||
t.Errorf("get calls = %d, want %d", getCalls, spriteControlRetries)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildExtractScript(t *testing.T) {
|
||||
script := buildExtractScript(12345)
|
||||
|
||||
|
||||
@@ -210,6 +210,46 @@ describe("session.models_updated config metadata", () => {
|
||||
],
|
||||
});
|
||||
});
|
||||
|
||||
it("preserves the hydrated config baseline when a provider snapshot omits it", () => {
|
||||
const configBaseline = {
|
||||
model: providerModelId,
|
||||
[reasoningOptionId]: "medium",
|
||||
};
|
||||
const store = makeStore({
|
||||
sessionModels: {
|
||||
bySessionId: {
|
||||
"session-1": {
|
||||
currentModelId: providerModelId,
|
||||
models: [],
|
||||
configOptions: [],
|
||||
configBaseline,
|
||||
},
|
||||
},
|
||||
} as AppState["sessionModels"],
|
||||
});
|
||||
const handler = registerSessionModelsHandlers(store)["session.models_updated"]!;
|
||||
|
||||
handler(
|
||||
makeMessage(
|
||||
makePayload(providerModelId, {
|
||||
config_options: [
|
||||
{
|
||||
type: "select",
|
||||
id: reasoningOptionId,
|
||||
name: reasoningOptionName,
|
||||
current_value: "medium",
|
||||
options: [{ value: "medium", name: "Medium" }],
|
||||
},
|
||||
],
|
||||
}),
|
||||
),
|
||||
);
|
||||
|
||||
expect(store.getState().sessionModels.bySessionId["session-1"].configBaseline).toEqual(
|
||||
configBaseline,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("session.models_updated persisted runtime hydration", () => {
|
||||
|
||||
@@ -137,7 +137,10 @@ export function registerSessionModelsHandlers(store: StoreApi<AppState>): WsHand
|
||||
category: o.category,
|
||||
options: o.options,
|
||||
})),
|
||||
configBaseline: payload.config_baseline ?? persisted.baseline,
|
||||
configBaseline:
|
||||
payload.config_baseline ??
|
||||
persisted.baseline ??
|
||||
state.sessionModels.bySessionId[sessionId]?.configBaseline,
|
||||
});
|
||||
|
||||
clearStaleActiveModel(state, sessionId, acpModels);
|
||||
|
||||
@@ -0,0 +1,57 @@
|
||||
---
|
||||
spec: docs/specs/preview-sprites-transient-retry/spec.md
|
||||
created: 2026-07-31
|
||||
status: complete
|
||||
---
|
||||
|
||||
# Implementation Plan: Preview Sprites Transient Retry
|
||||
|
||||
## Overview
|
||||
|
||||
The failed preview job reached `CreateSprite` after a successful local build,
|
||||
then failed on the SDK HTTP client's 30-second timeout. Add bounded,
|
||||
context-aware retries inside the preview CLI's get-or-create boundary, where
|
||||
the operation is safe to resume and can reconcile an ambiguous create. Keep
|
||||
the GitHub Actions workflow unchanged so permanent build or credential errors
|
||||
remain visible on their first attempt.
|
||||
|
||||
## Backend
|
||||
|
||||
### Sprites control-plane retry
|
||||
|
||||
- `apps/backend/cmd/preview/sprite_ops.go`: introduce transient-error
|
||||
classification and a bounded backoff helper. Retry discovery and creation
|
||||
with fresh per-attempt contexts; use `Retry-After` from `sprites.APIError`
|
||||
where it is supplied. Re-read the named sprite after a transient create
|
||||
error before another create request.
|
||||
- Preserve the existing upload retry independently. Do not broaden retries to
|
||||
bundle build, extraction, service deployment, health checking, or GitHub
|
||||
metadata updates.
|
||||
|
||||
## Tests
|
||||
|
||||
- **What:** a transient get error retries and then returns the discovered
|
||||
sprite. **File:** `apps/backend/cmd/preview/sprite_ops_test.go`. **How:** a
|
||||
HTTP test server returns a transient failure followed by a valid SDK
|
||||
response.
|
||||
- **What:** a transient create error is reconciled by a subsequent get rather
|
||||
than a second create. **File:** `apps/backend/cmd/preview/sprite_ops_test.go`.
|
||||
**How:** HTTP test server scripts get/create responses through the real SDK
|
||||
client.
|
||||
- **What:** non-transient errors fail without retry, and exhausted transient
|
||||
failures return the final error. **File:**
|
||||
`apps/backend/cmd/preview/sprite_ops_test.go`. **How:** table-driven fake
|
||||
SDK-backed HTTP test cases, including typed `sprites.APIError` responses.
|
||||
|
||||
## Implementation Waves And Parallel Candidates
|
||||
|
||||
Wave 1:
|
||||
|
||||
- [x] [task-01-sprites-control-plane-retry](task-01-sprites-control-plane-retry.md)
|
||||
|
||||
## Risks
|
||||
|
||||
- `CreateSprite` can complete remotely before its client receives a response;
|
||||
re-reading the deterministic sprite name is required before retrying it.
|
||||
- Retry classification must unwrap SDK errors so wrapped timeout and API errors
|
||||
are handled correctly without masking permanent failures.
|
||||
@@ -0,0 +1,30 @@
|
||||
---
|
||||
id: "01-sprites-control-plane-retry"
|
||||
title: "Retry transient preview Sprites control-plane failures"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/preview-sprites-transient-retry/spec.md"
|
||||
---
|
||||
|
||||
# Task 01: Retry transient preview Sprites control-plane failures
|
||||
|
||||
- **Acceptance:** `getOrCreateSprite` retries bounded transient get/create
|
||||
failures with observable retry output and cancellation-aware backoff.
|
||||
- **Acceptance:** An ambiguous transient create is reconciled with a named
|
||||
sprite lookup before another create request; retryable API `429`/`5xx` and
|
||||
network failures are distinct from permanent client errors.
|
||||
- **Acceptance:** Focused Go tests cover retry success, ambiguous-create
|
||||
reconciliation, non-retryable failure, and retry exhaustion.
|
||||
- **Verification:** `cd apps/backend && go test -v ./cmd/preview`
|
||||
- **Files likely touched:** `apps/backend/cmd/preview/sprite_ops.go`,
|
||||
`apps/backend/cmd/preview/sprite_ops_test.go`.
|
||||
- **Dependencies:** None.
|
||||
- **Parallelism:** sequential.
|
||||
- **Inputs:** `docs/specs/preview-sprites-transient-retry/spec.md`,
|
||||
`docs/plans/preview-sprites-transient-retry/plan.md`, the existing upload
|
||||
retry in `sprite_ops.go`, and Sprites SDK `APIError` retry metadata.
|
||||
- **Output contract:** Report the red/green test evidence, retry semantics,
|
||||
changed files, final targeted test result, risks, and task/plan status
|
||||
update.
|
||||
@@ -0,0 +1,54 @@
|
||||
---
|
||||
status: building
|
||||
created: 2026-07-31
|
||||
owner: kandev
|
||||
---
|
||||
|
||||
# Preview Sprites Transient Retry
|
||||
|
||||
## Why
|
||||
|
||||
PR preview deployment can fail when the Sprites control plane briefly times
|
||||
out, even though the source build and preview configuration are valid. A
|
||||
maintainer should not need to rerun a whole workflow for a recoverable API
|
||||
failure.
|
||||
|
||||
## What
|
||||
|
||||
- Preview deployment retries a bounded number of transient Sprites
|
||||
get-or-create failures with visible backoff.
|
||||
- A retry is limited to transport timeouts, temporary network errors, HTTP
|
||||
`429`, and HTTP `5xx` responses. Authentication, authorization, validation,
|
||||
and other client errors fail immediately.
|
||||
- After a transient create failure, deployment re-reads the deterministic PR
|
||||
sprite name before another create attempt. If the first request actually
|
||||
created the sprite, deployment resumes without creating a duplicate.
|
||||
- Retries respect cancellation and do not retry the complete GitHub Actions
|
||||
workflow or repeat the frontend build.
|
||||
|
||||
## Failure modes
|
||||
|
||||
- After the retry budget is exhausted, deployment fails with the last Sprites
|
||||
operation error so the GitHub Actions log remains actionable.
|
||||
- A Sprites-provided `Retry-After` delay takes precedence over exponential
|
||||
backoff when present.
|
||||
|
||||
## Scenarios
|
||||
|
||||
- **GIVEN** Sprites times out while creating a preview sprite, **WHEN** the
|
||||
next control-plane attempt succeeds, **THEN** the preview deployment
|
||||
continues without a workflow rerun.
|
||||
- **GIVEN** Sprites creates a preview sprite but the response times out,
|
||||
**WHEN** deployment retries, **THEN** it finds and uses the existing named
|
||||
sprite instead of issuing another create.
|
||||
- **GIVEN** Sprites rejects a create request with an authentication or other
|
||||
non-retryable client error, **WHEN** deployment receives it, **THEN** the
|
||||
deployment fails immediately.
|
||||
- **GIVEN** all transient control-plane attempts fail, **WHEN** the retry
|
||||
budget is exhausted, **THEN** the deployment fails with the final error.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Retrying frontend builds, bundle packaging, or GitHub PR-description writes.
|
||||
- Changing Sprites account quotas or retrying preview cleanup.
|
||||
- Adding a GitHub Actions job-level retry wrapper.
|
||||
Reference in New Issue
Block a user