fix: retry transient preview deploy errors (#3843)

2026-09-21 15:59:20 +01:00
parent 10726dfe8e
commit a7f7e01af8
7 changed files with 359 additions and 27 deletions
@@ -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 == "" {
@@ -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 {
@@ -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)

@@ -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.
@@ -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`
@@ -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.

@@ -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.