diff --git a/apps/backend/cmd/preview/deploy.go b/apps/backend/cmd/preview/deploy.go index 471a94821e..bf9e701fba 100644 --- a/apps/backend/cmd/preview/deploy.go +++ b/apps/backend/cmd/preview/deploy.go @@ -220,18 +220,18 @@ func deployPreviewArtifact(ctx context.Context, tarPath, spritesToken, spriteNam // enablePublicURL sets the sprite's URL to public mode and returns the URL. // Accepts the already-open client from deployArtifacts to avoid a second auth handshake. func enablePublicURL(ctx context.Context, client *sprites.Client, spriteName string) (string, error) { - updateCtx, cancel := context.WithTimeout(ctx, spriteStepTimeout) - defer cancel() - - if err := client.UpdateURLSettings(updateCtx, spriteName, &sprites.URLSettings{Auth: "public"}); err != nil { + if err := retrySpriteControl(ctx, "update URL settings", func(updateCtx context.Context) error { + return client.UpdateURLSettings(updateCtx, spriteName, &sprites.URLSettings{Auth: "public"}) + }); err != nil { return "", fmt.Errorf("update URL settings: %w", err) } - getCtx, getCancel := context.WithTimeout(ctx, spriteStepTimeout) - defer getCancel() - - sprite, err := client.GetSprite(getCtx, spriteName) - if err != nil { + var sprite *sprites.Sprite + if err := retrySpriteControl(ctx, "get sprite URL", func(getCtx context.Context) error { + var err error + sprite, err = client.GetSprite(getCtx, spriteName) + return err + }); err != nil { return "", fmt.Errorf("get sprite URL: %w", err) } if sprite.URL == "" { diff --git a/apps/backend/cmd/preview/sprite_ops.go b/apps/backend/cmd/preview/sprite_ops.go index 5cbd53213a..a8933c0650 100644 --- a/apps/backend/cmd/preview/sprite_ops.go +++ b/apps/backend/cmd/preview/sprite_ops.go @@ -19,6 +19,7 @@ const ( spriteControlRetries = 3 spriteUploadRetries = 3 spriteBackoffInit = 700 * time.Millisecond + spriteRetryMaxDelay = 30 * time.Second ) func newSpriteClient(token string) *sprites.Client { @@ -90,11 +91,7 @@ func isTransientSpriteError(err error) bool { } 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 - } + delay := spriteRetryDelay(attempt, err) fmt.Fprintf(os.Stderr, " %s attempt %d failed (%v), retrying in %v...\n", operation, attempt, err, delay) select { @@ -105,6 +102,40 @@ func waitForSpriteRetry(ctx context.Context, operation string, attempt int, err } } +func spriteRetryDelay(attempt int, err error) time.Duration { + delay := spriteBackoffInit * time.Duration(1<<(attempt-1)) + if delay > spriteRetryMaxDelay { + delay = spriteRetryMaxDelay + } + var apiErr *sprites.APIError + if errors.As(err, &apiErr) && apiErr.GetRetryAfterSeconds() > 0 { + retryAfterSeconds := apiErr.GetRetryAfterSeconds() + maxRetryAfterSeconds := int(spriteRetryMaxDelay / time.Second) + if retryAfterSeconds > maxRetryAfterSeconds { + retryAfterSeconds = maxRetryAfterSeconds + } + delay = time.Duration(retryAfterSeconds) * time.Second + } + return delay +} + +func retrySpriteControl(ctx context.Context, operation string, action func(context.Context) error) error { + for attempt := 1; ; attempt++ { + stepCtx, cancel := context.WithTimeout(ctx, spriteStepTimeout) + err := action(stepCtx) + cancel() + if err == nil { + return nil + } + if !isTransientSpriteError(err) || attempt >= spriteControlRetries { + return err + } + if err := waitForSpriteRetry(ctx, operation, attempt, err); err != nil { + return err + } + } +} + // uploadBundle uploads the bundle tarball to the sprite via the Filesystem API. // Retries up to spriteUploadRetries times on transient errors with context-aware backoff. func uploadBundle(ctx context.Context, sprite *sprites.Sprite, tarPath string) error { diff --git a/apps/backend/cmd/preview/sprite_ops_test.go b/apps/backend/cmd/preview/sprite_ops_test.go index dc2c7486de..c4e3a9a853 100644 --- a/apps/backend/cmd/preview/sprite_ops_test.go +++ b/apps/backend/cmd/preview/sprite_ops_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" sprites "github.com/superfly/sprites-go" ) @@ -136,6 +137,142 @@ func TestGetOrCreateSpriteReturnsFinalTransientErrorAfterRetryBudget(t *testing. } } +func TestSpriteRetryDelayCapsRetryAfter(t *testing.T) { + err := &sprites.APIError{ + StatusCode: http.StatusServiceUnavailable, + RetryAfterSeconds: int(spriteRetryMaxDelay/time.Second) + 1, + } + + if got := spriteRetryDelay(1, err); got != spriteRetryMaxDelay { + t.Fatalf("spriteRetryDelay() = %v, want cap %v", got, spriteRetryMaxDelay) + } +} + +func TestEnablePublicURLRetriesTransientUpdate(t *testing.T) { + updateCalls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPut && r.URL.Path == "/v1/sprites/kandev-pr-2115": + updateCalls++ + if updateCalls == 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + case r.Method == http.MethodGet && r.URL.Path == "/v1/sprites/kandev-pr-2115": + _, _ = w.Write([]byte(`{"name":"kandev-pr-2115","url":"https://preview.example"}`)) + default: + t.Errorf("unexpected %s %s request", r.Method, r.URL.Path) + } + })) + t.Cleanup(server.Close) + + client := sprites.New("token", sprites.WithBaseURL(server.URL)) + t.Cleanup(func() { _ = client.Close() }) + + got, err := enablePublicURL(t.Context(), client, "kandev-pr-2115") + if err != nil { + t.Fatalf("enablePublicURL() error = %v", err) + } + if got != "https://preview.example" { + t.Fatalf("enablePublicURL() = %q, want preview URL", got) + } + if updateCalls != 2 { + t.Fatalf("URL settings update calls = %d, want 2", updateCalls) + } +} + +func TestEnablePublicURLReturnsErrorAfterRetryBudget(t *testing.T) { + updateCalls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPut && r.URL.Path == "/v1/sprites/kandev-pr-2115": + updateCalls++ + w.WriteHeader(http.StatusServiceUnavailable) + default: + t.Errorf("unexpected %s %s request", r.Method, r.URL.Path) + } + })) + t.Cleanup(server.Close) + + client := sprites.New("token", sprites.WithBaseURL(server.URL)) + t.Cleanup(func() { _ = client.Close() }) + + _, err := enablePublicURL(t.Context(), client, "kandev-pr-2115") + if err == nil { + t.Fatal("enablePublicURL() error = nil, want transient error after budget exhausted") + } + if updateCalls != spriteControlRetries { + t.Fatalf("URL settings update calls = %d, want %d", updateCalls, spriteControlRetries) + } +} + +func TestEnablePublicURLRetriesTransientSpriteLookup(t *testing.T) { + getCalls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPut && r.URL.Path == "/v1/sprites/kandev-pr-2115": + w.WriteHeader(http.StatusOK) + case r.Method == http.MethodGet && r.URL.Path == "/v1/sprites/kandev-pr-2115": + getCalls++ + if getCalls == 1 { + w.WriteHeader(http.StatusBadGateway) + return + } + _, _ = w.Write([]byte(`{"name":"kandev-pr-2115","url":"https://preview.example"}`)) + default: + t.Errorf("unexpected %s %s request", r.Method, r.URL.Path) + } + })) + t.Cleanup(server.Close) + + client := sprites.New("token", sprites.WithBaseURL(server.URL)) + t.Cleanup(func() { _ = client.Close() }) + + got, err := enablePublicURL(t.Context(), client, "kandev-pr-2115") + if err != nil { + t.Fatalf("enablePublicURL() error = %v", err) + } + if got != "https://preview.example" { + t.Fatalf("enablePublicURL() = %q, want preview URL", got) + } + if getCalls != 2 { + t.Fatalf("sprite GET calls = %d, want 2", getCalls) + } +} + +func TestEnablePublicURLDoesNotRetryPermanentUpdateError(t *testing.T) { + updateCalls := 0 + getCalls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPut && r.URL.Path == "/v1/sprites/kandev-pr-2115": + updateCalls++ + w.WriteHeader(http.StatusUnauthorized) + case r.Method == http.MethodGet && r.URL.Path == "/v1/sprites/kandev-pr-2115": + getCalls++ + _, _ = w.Write([]byte(`{"name":"kandev-pr-2115","url":"https://preview.example"}`)) + default: + t.Errorf("unexpected %s %s request", r.Method, r.URL.Path) + } + })) + t.Cleanup(server.Close) + + client := sprites.New("token", sprites.WithBaseURL(server.URL)) + t.Cleanup(func() { _ = client.Close() }) + + _, err := enablePublicURL(t.Context(), client, "kandev-pr-2115") + if err == nil { + t.Fatal("enablePublicURL() error = nil, want unauthorized error") + } + if updateCalls != 1 { + t.Fatalf("URL settings update calls = %d, want 1", updateCalls) + } + if getCalls != 0 { + t.Fatalf("sprite GET calls = %d, want 0 after update failure", getCalls) + } +} + func TestBuildExtractScript(t *testing.T) { script := buildExtractScript(12345) diff --git a/docs/plans/preview-sprites-transient-retry/plan.md b/docs/plans/preview-sprites-transient-retry/plan.md index cbd095145f..64134fc220 100644 --- a/docs/plans/preview-sprites-transient-retry/plan.md +++ b/docs/plans/preview-sprites-transient-retry/plan.md @@ -1,19 +1,22 @@ --- -spec: docs/specs/ui/requirements/preview-sprites-transient-retry.md created: 2026-07-31 status: complete +requirements: + - REQ-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001 +system_design: + - ../../specs/ui/system-design/preview-sprites-transient-retry.md +legacy_specs: [] --- # 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. +The failed preview job reached the Sprites control plane after a successful +local build, then failed on a transient provider error. Add bounded, +context-aware retries inside the preview CLI's idempotent control-plane +boundaries. Keep the GitHub Actions workflow unchanged so permanent build or +credential errors remain visible on their first attempt. ## Backend @@ -21,13 +24,20 @@ remain visible on their first attempt. - `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. + with fresh per-attempt contexts; use positive `Retry-After` values from + `sprites.APIError` where supplied, capped at 30 seconds. 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. +### Public URL configuration + +- `apps/backend/cmd/preview/deploy.go`: apply the control-plane retry policy to + `UpdateURLSettings` and the following `GetSprite` URL lookup. +- Repeating the public URL update is safe because each attempt requests the + same idempotent public state. Permanent client errors stop before the lookup. + ## Tests - **What:** a transient get error retries and then returns the discovered @@ -42,12 +52,17 @@ remain visible on their first attempt. 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. +- **What:** public URL update and sprite URL lookup recover from transient + provider responses while permanent update errors stop immediately. **File:** + `apps/backend/cmd/preview/sprite_ops_test.go`. **How:** an HTTP test server + drives the real SDK through 500, 502, 200, and 401 responses. ## Implementation Waves And Parallel Candidates Wave 1: - [x] [task-01-sprites-control-plane-retry](task-01-sprites-control-plane-retry.md) +- [x] [task-02-public-url-control-retry](task-02-public-url-control-retry.md) ## Risks @@ -55,3 +70,6 @@ Wave 1: 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. +- Repeating a public URL update must remain idempotent; do not extend this + policy to non-idempotent service or metadata operations without a separate + design review. diff --git a/docs/plans/preview-sprites-transient-retry/task-02-public-url-control-retry.md b/docs/plans/preview-sprites-transient-retry/task-02-public-url-control-retry.md new file mode 100644 index 0000000000..80e8bee1b0 --- /dev/null +++ b/docs/plans/preview-sprites-transient-retry/task-02-public-url-control-retry.md @@ -0,0 +1,72 @@ +--- +id: "02-public-url-control-retry" +title: "Retry transient public URL control calls" +status: done +wave: 1 +depends_on: + - "01-sprites-control-plane-retry" +plan: "plan.md" +requirements: + - REQ-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001 +acceptance_criteria: + - AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.2 + - AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.4 + - AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.8 + - AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.9 + - AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.10 +system_design: + - ../../specs/ui/system-design/preview-sprites-transient-retry.md +--- + +# Task 02: Retry transient public URL control calls + +## Summary + +Extend the existing Sprites control-plane retry policy to the idempotent public +URL update and the sprite URL lookup that follows it. Keep permanent client +errors fail-fast and leave build, upload, service, health, and GitHub metadata +operations outside this retry boundary. + +## Acceptance + +- A transient 500 or 502 during public URL configuration or URL lookup retries + and then returns the preview URL without rebuilding the artifact. +- A transient failure on every attempt stops at the shared retry budget, and a + provider `Retry-After` delay is capped at 30 seconds. +- A permanent 401 during public URL configuration makes one request, returns + the provider error, and does not request the sprite URL. +- The focused preview package test passes with race detection. + +## Verification + +```bash +cd apps/backend && go test -race ./cmd/preview -count=1 +``` + +## Files likely touched + +- `apps/backend/cmd/preview/deploy.go` +- `apps/backend/cmd/preview/sprite_ops.go` +- `apps/backend/cmd/preview/sprite_ops_test.go` + +## Dependencies + +Task 01 provides the shared Sprites transient-error classification and backoff +policy. + +## Risks + +The URL update is retried only because each attempt requests the same public +state. Do not reuse this helper for non-idempotent operations without defining +their reconciliation behavior. + +## Results + +Implemented the shared control retry wrapper for public URL configuration and +sprite URL lookup. Added real SDK-backed HTTP regression tests for transient +500/502 recovery, retry-budget exhaustion, bounded provider delays, and +permanent 401 fail-fast behavior. + +Verification passed: + +- `cd apps/backend && go test -race ./cmd/preview -count=1` diff --git a/docs/specs/ui/requirements/preview-sprites-transient-retry.md b/docs/specs/ui/requirements/preview-sprites-transient-retry.md index 943700605c..e275494f21 100644 --- a/docs/specs/ui/requirements/preview-sprites-transient-retry.md +++ b/docs/specs/ui/requirements/preview-sprites-transient-retry.md @@ -27,6 +27,8 @@ PR preview deployment can fail when the Sprites control plane briefly times out, - **AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.6:** **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. - **AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.7:** **GIVEN** Sprites rejects a create request with an authentication or other non-retryable client error, **WHEN** deployment receives it, **THEN** the deployment fails immediately. - **AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.8:** **GIVEN** all transient control-plane attempts fail, **WHEN** the retry budget is exhausted, **THEN** the deployment fails with the final error. +- **AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.9:** Public URL configuration and the follow-up sprite URL lookup use the same bounded transient-error retry policy. +- **AC-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001.10:** **GIVEN** Sprites rejects public URL configuration with an authentication, authorization, validation, or other non-retryable client error, **WHEN** deployment receives it, **THEN** deployment fails without retrying or continuing to the URL lookup. ## Migrated source detail @@ -39,8 +41,9 @@ failure. ## What -- Preview deployment retries a bounded number of transient Sprites - get-or-create failures with visible backoff. +- Preview deployment retries a bounded number of transient Sprites control-plane + failures with visible backoff, including sprite lookup, creation, public URL + configuration, and the follow-up URL lookup. - 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. @@ -55,7 +58,8 @@ failure. - 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. + backoff when present and is capped at 30 seconds so provider input cannot + stall a preview deployment indefinitely. ## Scenarios @@ -68,6 +72,12 @@ failure. - **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** Sprites returns a transient error while enabling the public URL, + **WHEN** a later control-plane attempt succeeds, **THEN** deployment continues + without rebuilding or rerunning the workflow. +- **GIVEN** Sprites rejects public URL configuration with a permanent client + error, **WHEN** deployment receives it, **THEN** deployment fails immediately + and does not request the sprite URL. - **GIVEN** all transient control-plane attempts fail, **WHEN** the retry budget is exhausted, **THEN** the deployment fails with the final error. diff --git a/docs/specs/ui/system-design/preview-sprites-transient-retry.md b/docs/specs/ui/system-design/preview-sprites-transient-retry.md new file mode 100644 index 0000000000..9a9a5986a8 --- /dev/null +++ b/docs/specs/ui/system-design/preview-sprites-transient-retry.md @@ -0,0 +1,64 @@ +--- +status: current +system: ui +created: 2026-09-21 +requirements: + - REQ-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001 +--- + +# Preview Sprites transient retry system design + +## Purpose and boundaries + +The preview CLI owns the Sprites control-plane calls used to deploy a pull +request preview. It retries recoverable control-plane failures inside the CLI, +so a transient provider failure does not repeat the frontend build or the whole +GitHub Actions job. Bundle creation, service commands, health checks, and +GitHub description writes remain outside this retry boundary. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| REQ-UI-PREVIEW-SPRITES-TRANSIENT-RETRY-001 | Retry policy and deployment flow | + +## Components and responsibilities + +- `apps/backend/cmd/preview/sprite_ops.go` classifies transient errors and + provides bounded, context-aware control-plane retries. It recognizes + transport timeouts, temporary network errors, HTTP 429, and HTTP 5xx errors. +- `retrySpriteControl` creates a fresh two-minute operation context for each + attempt, emits the operation and backoff to stderr, honors a positive Sprites + SDK `Retry-After` value up to the 30-second retry-delay cap, and stops after + the shared control retry budget. +- `apps/backend/cmd/preview/deploy.go` uses the helper for the idempotent public + URL update and for the sprite URL lookup that follows it. + +## Retry policy and deployment flow + +The deploy command first uploads and extracts the bundle. It then updates the +deterministic PR sprite's URL settings to public mode. A transient failure in +that update is retried before deployment proceeds. After a successful update, +the CLI retrieves the sprite URL through the same policy. Repeating the URL +update is safe because the requested state is the same public setting. + +Each operation receives at most three attempts. Exponential backoff starts at +700 milliseconds and doubles between attempts. A positive provider +`Retry-After` delay replaces the calculated backoff, including when it is +shorter, and is capped at 30 seconds. Parent cancellation stops the backoff +and the operation. Permanent client errors return immediately with the +provider error. + +## Failure and recovery behavior + +If all attempts fail, the final Sprites error is returned to the deploy command +and the job fails with the operation context in its diagnostic. The CLI never +rebuilds the artifact, repeats the upload, or retries the GitHub workflow as a +consequence of a control-plane retry. + +## Verification + +The preview package tests use an HTTP test server with the real Sprites SDK to +verify transient update and lookup recovery, retry-budget exhaustion, permanent +update failure without an extra request, bounded `Retry-After` handling, and +the existing get-or-create retry behavior.