Places: Reuse pooled transport and log geocoding host only

internal/service/hub/places/cell.go

@@ -50,10 +50,11 @@ func Cell(id string, locale string) (result Location, err error) {
 	}
 
 	var r *http.Response
+	var reqUrl string
 
 	// Query the specified places service URLs.
 	for _, serviceUrl := range LocationServiceUrls {
-		reqUrl := fmt.Sprintf(serviceUrl, id)
+		reqUrl = fmt.Sprintf(serviceUrl, id)
 		if r, err = GetRequest(reqUrl, locale); err == nil {
 			break
 		}
@@ -61,7 +62,7 @@ func Cell(id string, locale string) (result Location, err error) {
 
 	// Failed?
 	if err != nil {
-		log.Errorf("places: %s (location request failed)", err.Error())
+		log.Errorf("places: location request to %s failed (%s)", serviceHost(reqUrl), safeError(err))
 		return result, err
 	} else if r == nil {
 		err = fmt.Errorf("location request could not be performed")

internal/service/hub/places/latlng.go

@@ -41,10 +41,11 @@ func LatLng(lat, lng float64, locale string) (result Location, err error) {
 	}
 
 	var r *http.Response
+	var reqUrl string
 
 	// Query the specified places service URLs.
 	for _, serviceUrl := range ReverseServiceUrls {
-		reqUrl := fmt.Sprintf("%s?%s", serviceUrl, params)
+		reqUrl = fmt.Sprintf("%s?%s", serviceUrl, params)
 		if r, err = GetRequest(reqUrl, locale); err == nil {
 			break
 		}
@@ -52,7 +53,7 @@ func LatLng(lat, lng float64, locale string) (result Location, err error) {
 
 	// Failed?
 	if err != nil {
-		log.Errorf("places: %s (location request failed)", err.Error())
+		log.Errorf("places: location request to %s failed (%s)", serviceHost(reqUrl), safeError(err))
 		return result, err
 	} else if r == nil {
 		err = fmt.Errorf("location request could not be performed")

internal/service/hub/places/request.go

@@ -2,19 +2,73 @@ package places
 
 import (
 	"crypto/sha1" //nolint:gosec // required for upstream signature scheme
+	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
+	"strings"
 	"time"
 
 	"github.com/photoprism/photoprism/pkg/http/header"
 	"github.com/photoprism/photoprism/pkg/http/safe"
 )
 
-// GetRequest fetches the cell ID data from the service URL.
+// sharedTransport is a connection-pooling transport reused across all geocoding
+// requests. The idle-connection pool lives in the transport, not the client, so
+// a large index keeps warm connections instead of a fresh TLS handshake per
+// lookup — while each request still uses its own client for a request-scoped
+// timeout (see GetRequest).
+var sharedTransport = newTransport()
+
+// newTransport returns an http.Transport tuned for many short geocoding
+// requests. It keeps a larger idle-connection pool than the stdlib default to
+// reduce TLS-handshake churn, while leaving IdleConnTimeout below the backend's
+// so the client closes idle connections first and avoids the reuse race.
+func newTransport() *http.Transport {
+	t := http.DefaultTransport.(*http.Transport).Clone()
+	t.MaxIdleConns = 64
+	t.MaxIdleConnsPerHost = 16
+	t.IdleConnTimeout = 90 * time.Second
+	return t
+}
+
+// serviceHost returns the host of a request URL for UI-visible logs. It drops
+// the query so request coordinates never reach those logs; if the URL cannot be
+// parsed into a host, it still strips any query string rather than falling back
+// to the raw value.
+func serviceHost(reqUrl string) string {
+	if u, err := url.Parse(reqUrl); err == nil && u.Host != "" {
+		return u.Host
+	}
+	if i := strings.IndexByte(reqUrl, '?'); i >= 0 {
+		return reqUrl[:i]
+	}
+	return reqUrl
+}
+
+// safeError returns an error message safe for UI-visible logs. A failed request
+// yields a *url.Error whose Error() embeds the full request URL — including the
+// geocoding coordinates — so for that type only the underlying transport error
+// is returned; other errors are returned unchanged.
+func safeError(err error) string {
+	var urlErr *url.Error
+	if errors.As(err, &urlErr) && urlErr.Err != nil {
+		return urlErr.Err.Error()
+	}
+	if err != nil {
+		return err.Error()
+	}
+	return ""
+}
+
+// GetRequest fetches data from the specified geocoding service URL. Because
+// these are idempotent GET requests, it retries transient connection-level
+// failures up to Retries times with a linear backoff before giving up.
 func GetRequest(reqUrl string, locale string) (r *http.Response, err error) {
 	var req *http.Request
 
-	// Log request URL.
+	// Full request URL at trace level only; trace is developer-only and not
+	// surfaced in the UI, unlike the host-only error logs.
 	log.Tracef("places: sending request to %s", reqUrl)
 
 	if _, parseErr := safe.URL(reqUrl); parseErr != nil {
@@ -26,7 +80,7 @@ func GetRequest(reqUrl string, locale string) (r *http.Response, err error) {
 
 	// Ok?
 	if err != nil {
-		log.Errorf("places: %s", err.Error())
+		log.Errorf("places: request to %s failed (%s)", serviceHost(reqUrl), safeError(err))
 		return r, err
 	}
 
@@ -48,28 +102,27 @@ func GetRequest(reqUrl string, locale string) (r *http.Response, err error) {
 		req.Header.Set("X-Signature", fmt.Sprintf("%x", sha1.Sum([]byte(Key+reqUrl+Secret)))) //nolint:gosec // upstream expects SHA1
 	}
 
-	// Create new http.Client.
-	//
-	// NOTE: Timeout specifies a time limit for requests made by
-	// this Client. The timeout includes connection time, any
-	// redirects, and reading the response body. The timer remains
-	// running after GetRequest, Head, Post, or Do return and will
-	// interrupt reading of the Response.Body.
-	client := &http.Client{Timeout: 60 * time.Second}
+	// Per-request client sharing the pooled transport. The client stays per
+	// request because Client.Timeout covers reading the response body and keeps
+	// running after Do returns, so the deadline must be scoped to this request;
+	// the connection pool lives in sharedTransport, so lookups still reuse warm
+	// connections.
+	client := &http.Client{Timeout: 60 * time.Second, Transport: sharedTransport}
 
-	// Perform request.
+	// Retry transient connection-level failures on this idempotent GET with a
+	// linear backoff.
 	for i := 0; i < Retries; i++ {
 		// #nosec G704 reqUrl is parsed and scheme-validated above.
-		r, err = client.Do(req)
-
-		// Ok?
-		if err == nil {
+		if r, err = client.Do(req); err == nil {
 			return r, nil
 		}
 
-		// Wait before trying again?
-		if RetryDelay.Nanoseconds() > 0 {
-			time.Sleep(RetryDelay)
+		// Full URL and raw error at trace level for developer troubleshooting.
+		log.Tracef("places: request to %s failed (%s)", reqUrl, err)
+
+		// Back off before the next attempt, skipping the wait after the last try.
+		if RetryDelay > 0 && i < Retries-1 {
+			time.Sleep(RetryDelay * time.Duration(i+1))
 		}
 	}
 

internal/service/hub/places/request_test.go

@@ -1,8 +1,12 @@
 package places
 
 import (
+	"errors"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
+	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 )
@@ -69,4 +73,117 @@ func TestGetRequest(t *testing.T) {
 			t.Fatal("expected URL parse error")
 		}
 	})
+	t.Run("RetryOnConnectionError", func(t *testing.T) {
+		prevRetries := Retries
+		prevDelay := RetryDelay
+		prevAgent := UserAgent
+		prevKey := Key
+		prevSecret := Secret
+		defer func() {
+			Retries = prevRetries
+			RetryDelay = prevDelay
+			UserAgent = prevAgent
+			Key = prevKey
+			Secret = prevSecret
+		}()
+
+		Retries = 3
+		RetryDelay = 0
+		UserAgent = "PhotoPrism/TestSuite"
+		Key = ""
+		Secret = ""
+
+		var attempts int32
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			// Drop the connection on the first attempt to simulate a transient
+			// connection-level failure, then answer normally on the retry.
+			if atomic.AddInt32(&attempts, 1) == 1 {
+				if hj, ok := w.(http.Hijacker); ok {
+					if conn, _, hjErr := hj.Hijack(); hjErr == nil {
+						_ = conn.Close()
+						return
+					}
+				}
+			}
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer server.Close()
+
+		resp, err := GetRequest(server.URL+"/v1/reverse", "en")
+		if err != nil {
+			t.Fatalf("expected success after retry, got %v", err)
+		}
+		if resp == nil {
+			t.Fatal("expected response")
+		}
+		_ = resp.Body.Close()
+		if got := atomic.LoadInt32(&attempts); got < 2 {
+			t.Fatalf("expected at least 2 attempts (a retry), got %d", got)
+		}
+	})
+}
+
+// TestServiceHost verifies host extraction and query stripping for log safety.
+func TestServiceHost(t *testing.T) {
+	t.Run("WithQuery", func(t *testing.T) {
+		if got := serviceHost("https://places.photoprism.app/v1/reverse?lat=52.520000&lng=13.405000"); got != "places.photoprism.app" {
+			t.Fatalf("expected host without query, got %q", got)
+		}
+	})
+	t.Run("PathOnly", func(t *testing.T) {
+		if got := serviceHost("https://places.photoprism.xyz/v1/location/1e95998417cc"); got != "places.photoprism.xyz" {
+			t.Fatalf("expected host, got %q", got)
+		}
+	})
+	t.Run("HostlessStripsQuery", func(t *testing.T) {
+		// No host to extract, but the query (coordinates) must still be dropped.
+		if got := serviceHost("/v1/reverse?lat=52.520000&lng=13.405000"); got != "/v1/reverse" {
+			t.Fatalf("expected query stripped, got %q", got)
+		}
+	})
+	t.Run("Unparseable", func(t *testing.T) {
+		if got := serviceHost("://nope"); got != "://nope" {
+			t.Fatalf("expected raw fallback, got %q", got)
+		}
+	})
+}
+
+// TestSafeError verifies that a *url.Error's embedded request URL is dropped.
+func TestSafeError(t *testing.T) {
+	t.Run("StripsURLError", func(t *testing.T) {
+		err := &url.Error{Op: "Get", URL: "https://places.photoprism.app/v1/reverse?lat=52.520000&lng=13.405000", Err: errors.New("tls: EOF")}
+		got := safeError(err)
+		if got != "tls: EOF" {
+			t.Fatalf("expected underlying error only, got %q", got)
+		}
+		if strings.Contains(got, "lat=") || strings.Contains(got, "reverse") {
+			t.Fatalf("coordinates leaked into log message: %q", got)
+		}
+	})
+	t.Run("PlainError", func(t *testing.T) {
+		if got := safeError(errors.New("boom")); got != "boom" {
+			t.Fatalf("expected passthrough, got %q", got)
+		}
+	})
+	t.Run("Nil", func(t *testing.T) {
+		if got := safeError(nil); got != "" {
+			t.Fatalf("expected empty string, got %q", got)
+		}
+	})
+}
+
+func TestNewTransport(t *testing.T) {
+	tr := newTransport()
+	if tr == nil {
+		t.Fatal("expected transport")
+	}
+	if tr.MaxIdleConnsPerHost != 16 {
+		t.Fatalf("expected MaxIdleConnsPerHost 16, got %d", tr.MaxIdleConnsPerHost)
+	}
+	if tr.MaxIdleConns != 64 {
+		t.Fatalf("expected MaxIdleConns 64, got %d", tr.MaxIdleConns)
+	}
+	if tr.IdleConnTimeout != 90*time.Second {
+		t.Fatalf("expected IdleConnTimeout 90s, got %s", tr.IdleConnTimeout)
+	}
 }

internal/service/hub/places/search.go

@@ -48,10 +48,11 @@ func Search(q, locale string, count int) (results SearchResults, err error) {
 	}
 
 	var r *http.Response
+	var reqUrl string
 
 	// Query the specified places service URLs.
 	for _, serviceUrl := range SearchServiceUrls {
-		reqUrl := fmt.Sprintf("%s?%s", serviceUrl, params)
+		reqUrl = fmt.Sprintf("%s?%s", serviceUrl, params)
 		if r, err = GetRequest(reqUrl, locale); err == nil {
 			break
 		}
@@ -59,7 +60,7 @@ func Search(q, locale string, count int) (results SearchResults, err error) {
 
 	// Failed?
 	if err != nil {
-		log.Errorf("places: %s (search request failed)", err.Error())
+		log.Errorf("places: search request to %s failed (%s)", serviceHost(reqUrl), safeError(err))
 		return results, err
 	} else if r == nil {
 		err = fmt.Errorf("search request could not be performed")