From c5056f562e42818736f58cd0a898707d96195a1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 31 Jul 2026 23:20:06 +0100 Subject: [PATCH] fix(preview): retry transient sprites errors (#2116) --- apps/backend/cmd/preview/sprite_ops.go | 92 +++++++++--- apps/backend/cmd/preview/sprite_ops_test.go | 131 ++++++++++++++++++ .../lib/ws/handlers/session-models.test.ts | 40 ++++++ apps/web/lib/ws/handlers/session-models.ts | 5 +- .../preview-sprites-transient-retry/plan.md | 57 ++++++++ .../task-01-sprites-control-plane-retry.md | 30 ++++ .../preview-sprites-transient-retry/spec.md | 54 ++++++++ 7 files changed, 390 insertions(+), 19 deletions(-) create mode 100644 docs/plans/preview-sprites-transient-retry/plan.md create mode 100644 docs/plans/preview-sprites-transient-retry/task-01-sprites-control-plane-retry.md create mode 100644 docs/specs/preview-sprites-transient-retry/spec.md diff --git a/apps/backend/cmd/preview/sprite_ops.go b/apps/backend/cmd/preview/sprite_ops.go index 787360f7c4..92e4c8914f 100644 --- a/apps/backend/cmd/preview/sprite_ops.go +++ b/apps/backend/cmd/preview/sprite_ops.go @@ -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. diff --git a/apps/backend/cmd/preview/sprite_ops_test.go b/apps/backend/cmd/preview/sprite_ops_test.go index 8b2b30f32b..76af022cbe 100644 --- a/apps/backend/cmd/preview/sprite_ops_test.go +++ b/apps/backend/cmd/preview/sprite_ops_test.go @@ -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) diff --git a/apps/web/lib/ws/handlers/session-models.test.ts b/apps/web/lib/ws/handlers/session-models.test.ts index 921156b48f..0a3c22ec53 100644 --- a/apps/web/lib/ws/handlers/session-models.test.ts +++ b/apps/web/lib/ws/handlers/session-models.test.ts @@ -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", () => { diff --git a/apps/web/lib/ws/handlers/session-models.ts b/apps/web/lib/ws/handlers/session-models.ts index 064a522b92..0a685a4b11 100644 --- a/apps/web/lib/ws/handlers/session-models.ts +++ b/apps/web/lib/ws/handlers/session-models.ts @@ -137,7 +137,10 @@ export function registerSessionModelsHandlers(store: StoreApi): 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); diff --git a/docs/plans/preview-sprites-transient-retry/plan.md b/docs/plans/preview-sprites-transient-retry/plan.md new file mode 100644 index 0000000000..d3fb27a361 --- /dev/null +++ b/docs/plans/preview-sprites-transient-retry/plan.md @@ -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. diff --git a/docs/plans/preview-sprites-transient-retry/task-01-sprites-control-plane-retry.md b/docs/plans/preview-sprites-transient-retry/task-01-sprites-control-plane-retry.md new file mode 100644 index 0000000000..7e5a0a037c --- /dev/null +++ b/docs/plans/preview-sprites-transient-retry/task-01-sprites-control-plane-retry.md @@ -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. diff --git a/docs/specs/preview-sprites-transient-retry/spec.md b/docs/specs/preview-sprites-transient-retry/spec.md new file mode 100644 index 0000000000..e6b72d5aa7 --- /dev/null +++ b/docs/specs/preview-sprites-transient-retry/spec.md @@ -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.