internal/api: set client error cache duration

Establish a short cache duration for 4xx client errors to prevent
redundant calls on errors such as package not found or bad request.

Change-Id: I32dc5880d14955f081caaa6e6bbc9d951fad1adb
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/805020
Auto-Submit: Ethan Lee <ethanalee@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
kokoro-CI: kokoro <noreply+kokoro@google.com>

internal/api/api.go

diff --git a/internal/api/api.go b/internal/api/api.go
index 0673825..b9e9908 100644
--- a/internal/api/api.go
+++ b/internal/api/api.go
@@ -802,8 +802,10 @@
 	longCacheDur = 3 * time.Hour
 	// The information on some pages can change relatively quickly.
 	shortCacheDur = 1 * time.Hour
-	// Errors should not be cached.
+	// Server errors (5xx) and transient errors should not be cached.
 	noCache = time.Duration(0)
+	// 4xx Client errors (such as 404 Not Found or 400 Bad Request) are cached for a short period.
+	clientErrorCacheDur = 10 * time.Minute
 )
 
 func serveJSON(w http.ResponseWriter, status int, data any, cacheDur time.Duration) error {
@@ -835,7 +837,14 @@
 		}
 	}
 	log.Errorf(r.Context(), "API error %d: %v", aerr.Code, aerr)
-	return serveJSON(w, aerr.Code, aerr, noCache)
+	cacheDur := noCache
+	// Cache deterministic 4xx client errors (e.g. 404 Not Found, 400 Bad Request) for a short duration.
+	// Exclude transient 4xx errors like 429 Too Many Requests (which would lock out rate-limited users after quota reset)
+	// and 5xx server errors (which should recover instantly as soon as backend issues resolve).
+	if aerr.Code >= 400 && aerr.Code < 500 && aerr.Code != http.StatusTooManyRequests && aerr.Code != http.StatusRequestTimeout {
+		cacheDur = clientErrorCacheDur
+	}
+	return serveJSON(w, aerr.Code, aerr, cacheDur)
 }
 
 // paginate returns a paginated response for the given list of items and pagination parameters.

internal/api/api_test.go

diff --git a/internal/api/api_test.go b/internal/api/api_test.go
index d99de33..5f8e171 100644
--- a/internal/api/api_test.go
+++ b/internal/api/api_test.go
@@ -6,11 +6,14 @@
 
 import (
 	"encoding/json"
+	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
 	"golang.org/x/pkgsite/internal"
+	"golang.org/x/pkgsite/internal/derrors"
 	"golang.org/x/pkgsite/internal/osv"
 	"golang.org/x/pkgsite/internal/testing/fakedatasource"
 	"golang.org/x/pkgsite/internal/vuln"
@@ -221,3 +224,45 @@
 		})
 	}
 }
+
+func TestServeErrorCacheControl(t *testing.T) {
+	for _, test := range []struct {
+		name       string
+		err        error
+		wantStatus int
+		wantCC     string
+	}{
+		{
+			name:       "404 not found is cached for 10 min",
+			err:        fmt.Errorf("module not found: %w", derrors.NotFound),
+			wantStatus: http.StatusNotFound,
+			wantCC:     "public, max-age=600",
+		},
+		{
+			name:       "400 bad request is cached for 10 min",
+			err:        BadRequest("invalid query", "details"),
+			wantStatus: http.StatusBadRequest,
+			wantCC:     "public, max-age=600",
+		},
+		{
+			name:       "500 internal error is not cached",
+			err:        errors.New("db failure"),
+			wantStatus: http.StatusInternalServerError,
+			wantCC:     "no-store",
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			r := httptest.NewRequest("GET", "/v1beta/module/nonexistent", nil)
+			w := httptest.NewRecorder()
+			if err := ServeError(w, r, test.err); err != nil {
+				t.Fatal(err)
+			}
+			if w.Code != test.wantStatus {
+				t.Errorf("status = %d, want %d", w.Code, test.wantStatus)
+			}
+			if got := w.Header().Get("Cache-Control"); got != test.wantCC {
+				t.Errorf("Cache-Control = %q, want %q", got, test.wantCC)
+			}
+		})
+	}
+}