From 67205eb7a9c8fd58b5faf340ae8d8f155060baa9 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 11 Nov 2026 21:13:39 +0000 Subject: [PATCH] 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 | 3 + internal/handlers/image.go | 4 +- .../handlers/image_errors_internal_test.go | 8 ++ internal/httpfetcher/httpfetcher.go | 16 +++- .../request_failed_internal_test.go | 75 +++++++++++++++++++ 5 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 internal/httpfetcher/request_failed_internal_test.go diff --git a/README.md b/README.md index 9522af0..9917d91 100644 --- a/README.md +++ b/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 diff --git a/internal/handlers/image.go b/internal/handlers/image.go index ff11b6e..108e3e6 100644 --- a/internal/handlers/image.go +++ b/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" diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index c8c2a33..360a6f1 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/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}, diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 7f8a8f7..d5f97a6 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/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 diff --git a/internal/httpfetcher/request_failed_internal_test.go b/internal/httpfetcher/request_failed_internal_test.go new file mode 100644 index 0000000..fd176f6 --- /dev/null +++ b/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) + } + }) + } +}