diff --git a/frontend/server/internal/content/content_test.go b/frontend/server/internal/content/content_test.go index ef90a28..1d199ea 100644 --- a/frontend/server/internal/content/content_test.go +++ b/frontend/server/internal/content/content_test.go @@ -19,6 +19,7 @@ import ( "path/filepath" "strings" "testing" + "time" "thermograph/frontend/internal/config" "thermograph/frontend/internal/contentapi" @@ -474,6 +475,116 @@ func TestMonthCtx(t *testing.T) { } } +// --- fetchWithCity ----------------------------------------------------------- + +func TestFetchWithCityHappyPath(t *testing.T) { + api := &fakeAPI{ + city: func(slug, _ string) (*contentapi.CityPayload, error) { + return &contentapi.CityPayload{City: contentapi.CityInfo{Slug: slug, Name: "Testville"}}, nil + }, + } + primary, city, err := fetchWithCity(api, "testville", func() (string, error) { return "primary-value", nil }) + if err != nil { + t.Fatal(err) + } + if primary != "primary-value" { + t.Errorf("primary = %q", primary) + } + if city.Slug != "testville" || city.Name != "Testville" { + t.Errorf("city = %+v", city) + } +} + +// When only primary fails, its error must win even though City() also ran +// (and, here, succeeded) -- matching the old sequential code's behavior, +// where City() was never even called once primary had already failed. +func TestFetchWithCityPrimaryErrorWins(t *testing.T) { + primaryErr := notFound("no such month") + api := &fakeAPI{ + city: func(slug, _ string) (*contentapi.CityPayload, error) { + return &contentapi.CityPayload{City: contentapi.CityInfo{Slug: slug}}, nil + }, + } + _, _, err := fetchWithCity(api, "testville", func() (string, error) { return "", primaryErr }) + if err != primaryErr { + t.Errorf("err = %v, want the primary error", err) + } +} + +// When primary succeeds but City() fails, City's error must surface -- same +// as the old sequential code (it called City() second and returned whatever +// that call did). +func TestFetchWithCityCityErrorSurfaces(t *testing.T) { + cityErr := notFound("no such city") + api := &fakeAPI{ + city: func(_, _ string) (*contentapi.CityPayload, error) { return nil, cityErr }, + } + _, _, err := fetchWithCity(api, "testville", func() (string, error) { return "primary-value", nil }) + if err != cityErr { + t.Errorf("err = %v, want the city error", err) + } +} + +// When BOTH fail, primary's error still wins -- the same priority as the +// single-failure case above, just with both goroutines erroring. +func TestFetchWithCityBothErrorPrimaryWins(t *testing.T) { + primaryErr, cityErr := notFound("primary"), notFound("city") + api := &fakeAPI{ + city: func(_, _ string) (*contentapi.CityPayload, error) { return nil, cityErr }, + } + _, _, err := fetchWithCity(api, "testville", func() (string, error) { return "", primaryErr }) + if err != primaryErr { + t.Errorf("err = %v, want the primary error (city error must not win)", err) + } +} + +// Proves the two calls actually run CONCURRENTLY rather than sequentially -- +// deterministically, via a rendezvous, not by timing (which would be flaky +// under CI load). Each fake blocks until BOTH have started; if fetchWithCity +// ran them sequentially, the second would never start until the first +// returned, and this would deadlock and fail on the test's own timeout. +func TestFetchWithCityRunsConcurrently(t *testing.T) { + started := make(chan struct{}, 2) + release := make(chan struct{}) + rendezvous := func() { + started <- struct{}{} + <-release + } + + api := &fakeAPI{ + city: func(_, _ string) (*contentapi.CityPayload, error) { + rendezvous() + return &contentapi.CityPayload{}, nil + }, + } + + done := make(chan struct{}) + go func() { + fetchWithCity(api, "testville", func() (string, error) { + rendezvous() + return "", nil + }) + close(done) + }() + + // Both goroutines must reach the rendezvous before either can proceed -- + // only possible if they were launched concurrently. + for range 2 { + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for both calls to start -- they are not running concurrently") + } + } + close(release) + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("fetchWithCity did not return after release") + } +} + func TestMonthCtxUnknownLabelIsError(t *testing.T) { h := newHandlers(t, fixtureAPI(t)) j := loadFixture[contentapi.MonthPayload](t, "month") diff --git a/frontend/server/internal/content/pages.go b/frontend/server/internal/content/pages.go index 43eaedf..a508df0 100644 --- a/frontend/server/internal/content/pages.go +++ b/frontend/server/internal/content/pages.go @@ -7,6 +7,7 @@ import ( "html/template" "net/http" "strings" + "sync" "thermograph/frontend/internal/contentapi" "thermograph/frontend/internal/contentdata" @@ -165,6 +166,49 @@ func (h *Handlers) cityCtx(j *contentapi.CityPayload) (map[string]any, format.Un return ctx, unit, nil } +// fetchWithCity runs a page's primary API call and the shared City lookup +// concurrently instead of sequentially: both are independent single-slug +// fetches (City never depends on primary's result), so waiting for one to +// fully round-trip before even starting the other was pure latency with +// nothing to show for it. Concurrent instead cuts that leg to roughly +// whichever call is slower. +// +// If primary fails, its error wins even when City also fails or hasn't +// finished — matching the old sequential code's behavior exactly (it never +// even called City() once primary had already failed). The one real +// trade-off: City() is now ALWAYS launched, even on a request that's about +// to 404 from primary, so an invalid slug costs one extra (wasted, cheap) +// backend lookup it previously skipped. Worth it: a bad slug is the rare +// path; a good one is the common path this speeds up. +func fetchWithCity[T any](api API, slug string, primary func() (T, error)) (T, contentapi.CityInfo, error) { + var ( + pVal T + pErr error + cPay *contentapi.CityPayload + cErr error + group sync.WaitGroup + ) + group.Add(2) + go func() { + defer group.Done() + pVal, pErr = primary() + }() + go func() { + defer group.Done() + cPay, cErr = api.City(slug, "") // no origin: the Python called city(slug) bare here too + }() + group.Wait() + + if pErr != nil { + return pVal, contentapi.CityInfo{}, pErr + } + if cErr != nil { + var zero T + return zero, contentapi.CityInfo{}, cErr + } + return pVal, cPay.City, nil +} + // --- month page -------------------------------------------------------------- // MonthPage renders /climate/{slug}/{month} (content.py's month_page). @@ -176,17 +220,14 @@ func (h *Handlers) cityCtx(j *contentapi.CityPayload) (map[string]any, format.Un // Jinja iterated (label, value) tuples). func (h *Handlers) MonthPage(w http.ResponseWriter, r *http.Request) { slug, month := r.PathValue("slug"), r.PathValue("month") - j, err := h.api.CityMonth(slug, month) + j, city, err := fetchWithCity(h.api, slug, func() (*contentapi.MonthPayload, error) { + return h.api.CityMonth(slug, month) + }) if err != nil { h.apiError(w, err) return } - cp, err := h.api.City(slug, "") // no origin: the Python called city(slug) bare here - if err != nil { - h.apiError(w, err) - return - } - ctx, unit, err := h.monthCtx(j, cp.City) + ctx, unit, err := h.monthCtx(j, city) if err != nil { h.serverError(w, err) return @@ -308,17 +349,15 @@ func fmtPeriodSide(unit format.Unit, s contentapi.PeriodSide) map[string]any { // temperature metrics, nil otherwise, as the Python set them. func (h *Handlers) RecordsPage(w http.ResponseWriter, r *http.Request) { slug := r.PathValue("slug") - j, err := h.api.CityRecords(slug, origin(r)) + o := origin(r) + j, city, err := fetchWithCity(h.api, slug, func() (*contentapi.RecordsPayload, error) { + return h.api.CityRecords(slug, o) + }) if err != nil { h.apiError(w, err) return } - cp, err := h.api.City(slug, "") // no origin, same as month_page - if err != nil { - h.apiError(w, err) - return - } - ctx, unit, err := h.recordsCtx(j, cp.City) + ctx, unit, err := h.recordsCtx(j, city) if err != nil { h.serverError(w, err) return