From 6c94036bd61f1e46b7326c6f46400f10b8fa8a0e Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 05:05:27 +0000 Subject: [PATCH] refactor(server/api): share the Cache-Control table and name the confidence thresholds setCacheHeaders (container spaHandler) carried a byte-identical copy of the extension switch in cacheControlForExt (Lambda static path), so adding an extension to one gave the same asset a different cache policy per deployment mode. setCacheHeaders now calls cacheControlForExt. confidenceBucketFor inlined 200 / 3 / 50. They are now named constants documented as mirrors of pkg/exchange's ConfidenceHighSavingsUSD, ConfidenceHighMinCount and ConfidenceMediumSavingsUSD. Importing those needs a cloud-commitments-go/pkg bump past d11bdde, which is left out here. The count-0 clamp is kept and documented as a deliberate difference from the exchange ranking's neutral score. No behavior change. New table tests pin every Cache-Control value through both static paths and the exact confidence boundaries, including count 0 and negative counts; they pass before and after. Refs #343 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_recommendations.go | 25 +++++- internal/api/handler_recommendations_test.go | 16 ++++ internal/server/static.go | 22 +++--- internal/server/static_test.go | 81 ++++++++++++++++++++ 4 files changed, 127 insertions(+), 17 deletions(-) diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 0c2d33e75..aa1edfb16 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -371,23 +371,40 @@ func (h *Handler) buildRecommendationDetail(ctx context.Context, rec *config.Rec } } +// Confidence bucket cut-offs for the recommendation drawer badge. They must +// stay equal to exchange.ConfidenceHighSavingsUSD, exchange.ConfidenceHighMinCount +// and exchange.ConfidenceMediumSavingsUSD in cloud-commitments-go pkg/exchange, +// which score the reshape ranking the badge accompanies. Those exported +// constants are newer than the pkg version pinned in go.mod; once the pin is +// bumped past them, alias these to the exchange constants (#343). +const ( + confidenceHighSavingsUSD = 200.0 // USD per month, high bucket + confidenceHighMinCount = 3 // instances, high bucket + confidenceMediumSavingsUSD = 50.0 // USD per month, medium bucket +) + // confidenceBucketFor mirrors the heuristic that previously lived // client-side in frontend/src/recommendations.ts. Centralizing it on // the server lets future provider-specific tuning land in one place // without a frontend deploy. Thresholds intentionally match the original // shim 1:1 so the drawer label doesn't visibly shift on rollout. // -// high = ≥ $200/mo savings AND ≥ 3-instance fleet (both signals) -// medium = ≥ $50/mo savings (single signal) +// high = savings >= confidenceHighSavingsUSD AND count >= confidenceHighMinCount +// medium = savings >= confidenceMediumSavingsUSD // low = otherwise +// +// A count below 1 (missing) is clamped to 1, so savings alone decide between +// medium and low. This deliberately differs from the exchange ranking, which +// returns a neutral score for a zero count: a badge must always show a +// bucket, while a ranking score should stay neutral on missing data. func confidenceBucketFor(savings float64, count int) string { if count < 1 { count = 1 } - if savings >= 200 && count >= 3 { + if savings >= confidenceHighSavingsUSD && count >= confidenceHighMinCount { return "high" } - if savings >= 50 { + if savings >= confidenceMediumSavingsUSD { return "medium" } return "low" diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index 6395e124b..e19679a6a 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -807,6 +807,22 @@ func TestConfidenceBucketFor(t *testing.T) { {"medium on savings alone", 60, 1, "medium"}, {"low when neither threshold is met", 10, 1, "low"}, {"count clamped to >=1 — savings still drives bucket", 60, 0, "medium"}, + // Exact boundaries, pinned with literals so a change to the named + // thresholds fails here (#343). Values match pkg/exchange's + // ConfidenceHighSavingsUSD / ConfidenceHighMinCount / + // ConfidenceMediumSavingsUSD. + {"high at both thresholds", 200, 3, "high"}, + {"just below high savings", 199.99, 3, "medium"}, + {"just below high count", 200, 2, "medium"}, + {"medium at threshold", 50, 1, "medium"}, + {"just below medium", 49.99, 1, "low"}, + {"zero savings", 0, 10, "low"}, + // Count-0 handling: the badge clamps a missing count to 1 instead of + // going neutral like the exchange ranking does, so high savings with + // no count is medium (never high) and low savings stays low. + {"count zero with high savings is medium, not high", 10000, 0, "medium"}, + {"count zero with low savings is low", 10, 0, "low"}, + {"negative count is clamped like zero", 10000, -5, "medium"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/server/static.go b/internal/server/static.go index 80cebbabb..cc8d62c85 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -34,19 +34,12 @@ func (h *spaHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.ServeFile(w, r, filePath) // #nosec G703 -- filePath validated by resolveStaticFilePath: filepath.Abs + isPathContainedIn prevents directory traversal } -// setCacheHeaders sets Cache-Control based on file type. -// HTML files get no-cache; hashed assets (JS/CSS/images) get immutable long cache. +// setCacheHeaders sets Cache-Control based on file type. It delegates to +// cacheControlForExt, the single extension-to-policy table shared with the +// Lambda static path (serveStaticForLambda), so container and Lambda +// Function URL deployments cannot drift onto different cache policies. func setCacheHeaders(w http.ResponseWriter, urlPath string) { - ext := strings.ToLower(path.Ext(urlPath)) - switch ext { - case ".html": - w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate") - case ".js", ".css", ".woff", ".woff2", ".ttf", ".eot", - ".png", ".jpg", ".jpeg", ".gif", ".svg", ".ico", ".webp": - w.Header().Set("Cache-Control", "public, max-age=31536000, immutable") - default: - w.Header().Set("Cache-Control", "public, max-age=3600") - } + w.Header().Set("Cache-Control", cacheControlForExt(strings.ToLower(path.Ext(urlPath)))) } // isPathContainedIn reports whether absFile is at or under absDir, using a @@ -173,7 +166,10 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok return spaIndex(absDir, dir) } -// cacheControlForExt returns the Cache-Control header value for a file extension. +// cacheControlForExt returns the Cache-Control header value for a lower-cased +// file extension. HTML gets no-cache; hashed assets (JS/CSS/fonts/images) get +// an immutable long cache; everything else gets a one-hour cache. Both static +// paths (setCacheHeaders and serveStaticForLambda) use it. func cacheControlForExt(ext string) string { switch ext { case ".html": diff --git a/internal/server/static_test.go b/internal/server/static_test.go index f13b476ce..efb84a8a4 100644 --- a/internal/server/static_test.go +++ b/internal/server/static_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "os" + "path" "path/filepath" "strconv" "strings" @@ -81,6 +82,86 @@ func TestSetCacheHeaders(t *testing.T) { } } +// ----- cache policy pins (issue #343) ----- + +const ( + ccNoCache = "no-cache, no-store, must-revalidate" + ccImmutable = "public, max-age=31536000, immutable" + ccOneHour = "public, max-age=3600" +) + +// cachePolicyPins pins the exact Cache-Control value for every extension the +// policy table names, plus case variants and unlisted extensions that fall +// through to the default. The table must pass unchanged before and after +// setCacheHeaders was made to delegate to cacheControlForExt (#343). +var cachePolicyPins = []struct { + file string + want string +}{ + {"app.html", ccNoCache}, // not index.html: http.ServeFile 301s that to "/" + {"INDEX.HTML", ccNoCache}, + {"bundle.js", ccImmutable}, + {"style.css", ccImmutable}, + {"font.woff", ccImmutable}, + {"font.woff2", ccImmutable}, + {"font.ttf", ccImmutable}, + {"font.eot", ccImmutable}, + {"logo.png", ccImmutable}, + {"LOGO.PNG", ccImmutable}, + {"photo.jpg", ccImmutable}, + {"photo.jpeg", ccImmutable}, + {"anim.gif", ccImmutable}, + {"icon.svg", ccImmutable}, + {"favicon.ico", ccImmutable}, + {"hero.webp", ccImmutable}, + {"Mixed.Js", ccImmutable}, + {"data.json", ccOneHour}, + {"hero.avif", ccOneHour}, + {"bundle.js.map", ccOneHour}, + {"robots.txt", ccOneHour}, + {"LICENSE", ccOneHour}, +} + +// TestCachePolicyPinned asserts exact header values from both entry points: +// setCacheHeaders (container spaHandler) and cacheControlForExt (Lambda). +func TestCachePolicyPinned(t *testing.T) { + for _, tt := range cachePolicyPins { + t.Run(tt.file, func(t *testing.T) { + urlPath := "/assets.v2/" + tt.file + w := httptest.NewRecorder() + setCacheHeaders(w, urlPath) + testutil.AssertEqual(t, tt.want, w.Header().Get("Cache-Control")) + testutil.AssertEqual(t, tt.want, cacheControlForExt(strings.ToLower(path.Ext(urlPath)))) + }) + } +} + +// TestCachePolicyParity_ContainerAndLambda serves the same files through the +// container handler and the Lambda static path and requires an identical +// Cache-Control header from both, so a deployment mode cannot change the +// cache policy for an asset. +func TestCachePolicyParity_ContainerAndLambda(t *testing.T) { + files := map[string]string{} + for _, tt := range cachePolicyPins { + files[tt.file] = "x" + } + dir := makeStaticDir(t, files) + handler := spaFileServer(dir) + for _, tt := range cachePolicyPins { + t.Run(tt.file, func(t *testing.T) { + _, _, lambdaCC, found := serveStaticForLambda(dir, "/"+tt.file) + if !found { + t.Fatalf("serveStaticForLambda did not find %s", tt.file) + } + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/"+tt.file, http.NoBody)) + testutil.AssertEqual(t, http.StatusOK, rec.Code) + testutil.AssertEqual(t, tt.want, lambdaCC) + testutil.AssertEqual(t, tt.want, rec.Header().Get("Cache-Control")) + }) + } +} + // ----- isStaticPath ----- func TestIsStaticPath(t *testing.T) {