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, when the fetch failed before the upstream answered,
or when the fetcher refused a redirect. Such a failure is now
ErrInvalidHost or ErrUpstreamRequestFailed, which respondImageError
answers 502 "upstream error" and logs at warn level. A TLS handshake, an
address lookup or a connection that times out is now ErrUpstreamTimeout
and answers 504; its log line keeps the cause. A cancelled fetch or
address lookup is neither ErrUpstreamRequestFailed nor ErrInvalidHost.
README Routes lists these answers.

Model: opus-5-5

README.md

@@ -346,6 +346,8 @@ to any method the list does not give for it.
     - 503 in maintenance mode.
     - 504 when the address lookup of the upstream host, or of a host it
       redirects to, times out.
+    - 504 when the connection to the upstream host, or to a host it redirects
+      to, times out.
     - 504 when the upstream has not finished the TLS handshake within 10
       seconds.
     - 504 when the upstream has not sent its response headers within

internal/httpfetcher/httpfetcher.go

@@ -318,7 +318,8 @@ func (f *HTTPFetcher) Fetch(
 
 // upstreamRequestError returns the error Fetch reports when its upstream
 // request fails with err:
-//   - ErrUpstreamTimeout for a timeout.
+//   - err wrapped in ErrUpstreamTimeout for a timeout, so the log names what
+//     timed out.
 //   - err, still matching context.Canceled, when the caller cancelled the
 //     fetch.
 //   - err wrapped in ErrUpstreamRequestFailed otherwise.
@@ -331,8 +332,9 @@ func upstreamRequestError(err error) error {
 		errors.As(err, &netErr) && netErr.Timeout():
 		// A timeout other than the fetch timeout, such as the TLS
 		// handshake timeout, does not match context.DeadlineExceeded. An
-		// address lookup that timed out is ErrUpstreamTimeout already.
-		return ErrUpstreamTimeout
+		// address lookup or a connection that timed out is
+		// ErrUpstreamTimeout already, so its log line names that error twice.
+		return fmt.Errorf("%w: %w", ErrUpstreamTimeout, err)
 	case errors.Is(err, context.Canceled):
 		// The caller cancelled the fetch; the upstream did not fail
 		return fmt.Errorf("upstream request cancelled: %w", err)
@@ -544,13 +546,19 @@ func validateURL(ctx context.Context, fetchURL *neturl.URL, allowHTTP bool) erro
 }
 
 // addressLookupError returns the error for a failed address lookup of host:
-// ErrUpstreamTimeout when the lookup timed out, ErrInvalidHost otherwise.
+//   - ErrUpstreamTimeout when the lookup timed out.
+//   - err, still matching context.Canceled, when the lookup was cancelled.
+//   - ErrInvalidHost otherwise.
 func addressLookupError(host string, err error) error {
 	var netErr net.Error
 	if errors.As(err, &netErr) && netErr.Timeout() {
 		return fmt.Errorf("%w: address lookup of %s", ErrUpstreamTimeout, host)
 	}
 
+	if errors.Is(err, context.Canceled) {
+		return fmt.Errorf("address lookup cancelled: %w", err)
+	}
+
 	return fmt.Errorf("%w: %s", ErrInvalidHost, host)
 }
 
@@ -693,7 +701,10 @@ func dialSSRFSafe(
 	}
 
 	// Connect using the first valid IP
-	var dialer net.Dialer
+	var (
+		dialer      net.Dialer
+		lastDialErr error
+	)
 
 	for _, ip := range ips {
 		addr := net.JoinHostPort(ip.String(), port)
@@ -702,9 +713,18 @@ func dialSSRFSafe(
 		if err == nil {
 			return conn, nil
 		}
+
+		lastDialErr = err
 	}
 
-	return nil, fmt.Errorf("%w to %s", errConnectFailed, host)
+	// Only the last address tried decides whether the connection timed out
+	var netErr net.Error
+	if errors.As(lastDialErr, &netErr) && netErr.Timeout() {
+		return nil, fmt.Errorf("%w: connect to %s: %w",
+			ErrUpstreamTimeout, host, lastDialErr)
+	}
+
+	return nil, fmt.Errorf("%w to %s: %w", errConnectFailed, host, lastDialErr)
 }
 
 // limitedReader wraps a reader and refuses a body longer than remaining

internal/httpfetcher/request_failed_internal_test.go

@@ -13,8 +13,9 @@ import (
 // TestFetchFailedBeforeAnswer checks the error of a fetch whose connection
 // fails:
 //   - refused: ErrUpstreamRequestFailed.
-//   - timed out: ErrUpstreamTimeout, even for a timeout error that does not
-//     match context.DeadlineExceeded, such as the TLS handshake timeout.
+//   - timed out: ErrUpstreamTimeout, still matching the dial's error, even for
+//     a timeout error that does not match context.DeadlineExceeded, such as
+//     the TLS handshake timeout.
 //   - cancelled by its caller: context.Canceled, not ErrUpstreamRequestFailed.
 func TestFetchFailedBeforeAnswer(t *testing.T) {
 	t.Parallel()
@@ -22,7 +23,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 	tests := []struct {
 		name       string
 		dial       func(cancel context.CancelFunc) error
-		wantErr    error
+		wantErrs   []error
 		wantNotErr error
 	}{
 		{
@@ -30,7 +31,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			dial: func(context.CancelFunc) error {
 				return syscall.ECONNREFUSED
 			},
-			wantErr:    ErrUpstreamRequestFailed,
+			wantErrs:   []error{ErrUpstreamRequestFailed},
 			wantNotErr: context.Canceled,
 		},
 		{
@@ -38,7 +39,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			dial: func(context.CancelFunc) error {
 				return os.ErrDeadlineExceeded
 			},
-			wantErr:    ErrUpstreamTimeout,
+			wantErrs:   []error{ErrUpstreamTimeout, os.ErrDeadlineExceeded},
 			wantNotErr: ErrUpstreamRequestFailed,
 		},
 		{
@@ -48,7 +49,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 
 				return context.Canceled
 			},
-			wantErr:    context.Canceled,
+			wantErrs:   []error{context.Canceled},
 			wantNotErr: ErrUpstreamRequestFailed,
 		},
 	}
@@ -74,8 +75,10 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			_, 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)
+			for _, wantErr := range tt.wantErrs {
+				if !errors.Is(err, wantErr) {
+					t.Errorf("Fetch() error = %v, want %v", err, wantErr)
+				}
 			}
 
 			if errors.Is(err, tt.wantNotErr) {
@@ -146,3 +149,65 @@ func TestFetchAddressLookupTimedOut(t *testing.T) {
 		checkTimedOut(t, err)
 	})
 }
+
+// TestFetchConnectionTimedOut checks that a fetch whose connection in
+// dialSSRFSafe times out fails with ErrUpstreamTimeout, which the image
+// routes answer 504. The error must match neither ErrUpstreamRequestFailed
+// nor ErrInvalidHost, which they answer 502. A context whose deadline has
+// passed makes the connection time out without touching the network.
+func TestFetchConnectionTimedOut(t *testing.T) {
+	t.Parallel()
+
+	f := New(&Config{
+		Timeout:               30 * time.Second,
+		MaxResponseSize:       50 << 20,
+		MaxConnectionsPerHost: 20,
+		MaxConnections:        64,
+		DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
+			ctx, cancel := context.WithTimeout(ctx, 0)
+			defer cancel()
+
+			return dialSSRFSafe(ctx, network, addr, nil)
+		},
+	})
+
+	_, err := f.Fetch(testContext(t), upstreamSource("/image"), "", "")
+	t.Logf("Fetch() error: %v", err)
+
+	if !errors.Is(err, ErrUpstreamTimeout) {
+		t.Errorf("Fetch() error = %v, want ErrUpstreamTimeout", err)
+	}
+
+	if errors.Is(err, ErrUpstreamRequestFailed) || errors.Is(err, ErrInvalidHost) {
+		t.Errorf("Fetch() error = %v, must match neither "+
+			"ErrUpstreamRequestFailed nor ErrInvalidHost", err)
+	}
+}
+
+// TestFetchAddressLookupCancelled checks that a fetch whose address lookup
+// its caller cancelled fails with an error matching context.Canceled, not
+// ErrInvalidHost, which the image routes answer 502.
+func TestFetchAddressLookupCancelled(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithCancel(testContext(t))
+	cancel()
+
+	f := New(&Config{
+		Timeout:               30 * time.Second,
+		MaxResponseSize:       50 << 20,
+		MaxConnectionsPerHost: 20,
+		MaxConnections:        64,
+	})
+
+	_, err := f.Fetch(ctx, imageSourceOn("cdn.example.com"), "", "")
+	t.Logf("Fetch() error: %v", err)
+
+	if !errors.Is(err, context.Canceled) {
+		t.Errorf("Fetch() error = %v, want context.Canceled", err)
+	}
+
+	if errors.Is(err, ErrInvalidHost) {
+		t.Errorf("Fetch() error = %v, must not be ErrInvalidHost", err)
+	}
+}