Answer 502 for an upstream host with no address or a failed fetch (closes #367)

The image routes answered 500 and logged at error level when the upstream
host had no address, or when the fetch failed before the upstream
answered, such as on a refused connection. The fetcher now marks such a
failure with ErrUpstreamRequestFailed; a cancelled fetch is not marked.
respondImageError answers it, and ErrInvalidHost, 502 "upstream error"
and logs them at warn level. README Routes lists both under 502.

Model: opus-5-5

README.md

@@ -310,6 +310,9 @@ to any method the list does not give for it.
       neither the image nor its source in its cache.
     - 502 likewise for 5 minutes after a body whose first bytes are not those of
       its `Content-Type`.
+    - 502 when the upstream host has no address.
+    - 502 when the fetch fails before the upstream answers, such as on a refused
+      connection or a failed TLS handshake.
     - 503 when pixa is busy (see Limits).
     - 503 in maintenance mode.
     - 504 when the upstream has not sent its response headers within

internal/handlers/image.go

@@ -254,7 +254,9 @@ func (s *Handlers) respondImageError(
 	switch {
 	case errors.Is(err, httpfetcher.ErrSSRFBlocked):
 		status, message = http.StatusForbidden, "forbidden"
-	case errors.Is(err, httpfetcher.ErrUpstreamError):
+	case errors.Is(err, httpfetcher.ErrUpstreamError),
+		errors.Is(err, httpfetcher.ErrInvalidHost),
+		errors.Is(err, httpfetcher.ErrUpstreamRequestFailed):
 		status, message = http.StatusBadGateway, "upstream error"
 	case errors.Is(err, httpfetcher.ErrUpstreamTimeout):
 		status, message = http.StatusGatewayTimeout, "upstream timeout"

internal/handlers/image_errors_internal_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http/httptest"
 	"slices"
 	"strings"
+	"syscall"
 	"testing"
 	"testing/fstest"
 	"time"
@@ -461,6 +462,13 @@ func TestImageRoutes_SameErrorAnswers(t *testing.T) {
 		{"upstream error status",
 			fmt.Errorf("%w: status 404", httpfetcher.ErrUpstreamError),
 			http.StatusBadGateway, slog.LevelWarn},
+		{"upstream host with no address",
+			fmt.Errorf("%w: cdn.example.com", httpfetcher.ErrInvalidHost),
+			http.StatusBadGateway, slog.LevelWarn},
+		{"connection refused",
+			fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamRequestFailed,
+				syscall.ECONNREFUSED),
+			http.StatusBadGateway, slog.LevelWarn},
 		{"no response headers within the fetch timeout",
 			httpfetcher.ErrUpstreamTimeout, http.StatusGatewayTimeout,
 			slog.LevelWarn},

internal/httpfetcher/httpfetcher.go

@@ -69,7 +69,11 @@ var (
 	ErrInvalidContentType = errors.New("invalid or unsupported content type")
 	ErrUpstreamError      = errors.New("upstream server error")
 	ErrUpstreamTimeout    = errors.New("upstream request timeout")
-	ErrTooManyConnections = errors.New("too many concurrent upstream connections")
+	// ErrUpstreamRequestFailed is a fetch that failed before the upstream
+	// answered, other than by timeout or cancellation: a failed connection
+	// or TLS handshake, or a redirect the fetcher refuses.
+	ErrUpstreamRequestFailed = errors.New("upstream request failed")
+	ErrTooManyConnections    = errors.New("too many concurrent upstream connections")
 )
 
 // Internal fetcher errors.
@@ -283,11 +287,15 @@ func (f *HTTPFetcher) Fetch(
 	fetchDuration := time.Since(startTime)
 
 	if err != nil {
-		if errors.Is(err, context.DeadlineExceeded) {
+		switch {
+		case errors.Is(err, context.DeadlineExceeded):
 			return nil, ErrUpstreamTimeout
+		case errors.Is(err, context.Canceled):
+			// The caller cancelled the fetch; the upstream did not fail
+			return nil, fmt.Errorf("upstream request cancelled: %w", err)
+		default:
+			return nil, fmt.Errorf("%w: %w", ErrUpstreamRequestFailed, err)
 		}
-
-		return nil, fmt.Errorf("upstream request failed: %w", err)
 	}
 
 	// The deferred release gives the connection back, as no body follows

internal/httpfetcher/request_failed_internal_test.go

@@ -0,0 +1,75 @@
+package httpfetcher
+
+import (
+	"context"
+	"errors"
+	"net"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// TestFetchFailedBeforeAnswer checks that a fetch whose connection is refused
+// fails with ErrUpstreamRequestFailed, and that a fetch its caller cancels
+// while connecting fails with context.Canceled and not with
+// ErrUpstreamRequestFailed.
+func TestFetchFailedBeforeAnswer(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		dial       func(cancel context.CancelFunc) error
+		wantErr    error
+		wantNotErr error
+	}{
+		{
+			name: "connection refused",
+			dial: func(context.CancelFunc) error {
+				return syscall.ECONNREFUSED
+			},
+			wantErr:    ErrUpstreamRequestFailed,
+			wantNotErr: context.Canceled,
+		},
+		{
+			name: "cancelled while connecting",
+			dial: func(cancel context.CancelFunc) error {
+				cancel()
+
+				return context.Canceled
+			},
+			wantErr:    context.Canceled,
+			wantNotErr: ErrUpstreamRequestFailed,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(testContext(t))
+			defer cancel()
+
+			f := New(&Config{
+				Timeout:               30 * time.Second,
+				MaxResponseSize:       50 << 20,
+				AllowHTTP:             true,
+				MaxConnectionsPerHost: 20,
+				MaxConnections:        64,
+				DialContext: func(context.Context, string, string) (net.Conn, error) {
+					return nil, tt.dial(cancel)
+				},
+			})
+
+			_, err := f.Fetch(ctx, upstreamSource("/image"), "", "")
+			t.Logf("Fetch() error: %v", err)
+
+			if !errors.Is(err, tt.wantErr) {
+				t.Errorf("Fetch() error = %v, want %v", err, tt.wantErr)
+			}
+
+			if errors.Is(err, tt.wantNotErr) {
+				t.Errorf("Fetch() error = %v, must not be %v", err, tt.wantNotErr)
+			}
+		})
+	}
+}