From a0ff8fa5a7ed6d52aab3b0ee97ae28c833d3b379 Mon Sep 17 00:00:00 2001 From: AmanTahiliani Date: Sun, 12 Jul 2026 21:01:42 -0400 Subject: [PATCH] fix(#76): preserve pacing and empty-state truth --- internal/api/client.go | 8 ++- internal/api/pacing_test.go | 67 +++++++++++++++++++----- internal/web/api.go | 34 ++++++++++-- internal/web/championship_hub_test.go | 47 ++++++++++++++++- internal/web/component_freshness_test.go | 13 +++++ internal/web/freshness.go | 14 ++--- internal/web/freshness_test.go | 14 +++++ internal/web/replay.go | 2 +- internal/web/replay_test.go | 7 ++- 9 files changed, 176 insertions(+), 30 deletions(-) diff --git a/internal/api/client.go b/internal/api/client.go index 2951519..49d93b9 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -48,11 +48,9 @@ func (p *requestPacer) waitContext(ctx context.Context) error { select { case <-timer.C: case <-ctx.Done(): - // Return the unused reservation so repeated bounded enrichment - // cancellations do not leave pacing debt for later real requests. - p.mu.Lock() - p.next = p.next.Add(-p.interval) - p.mu.Unlock() + // Keep the unused reservation in the schedule. Blindly reclaiming an + // interval can collide with later callers that already reserved their + // wake times, releasing two requests simultaneously. return ctx.Err() } } diff --git a/internal/api/pacing_test.go b/internal/api/pacing_test.go index 20d3572..689ea13 100644 --- a/internal/api/pacing_test.go +++ b/internal/api/pacing_test.go @@ -3,6 +3,7 @@ package api import ( "context" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -50,25 +51,67 @@ func TestRequestPacerNilSafe(t *testing.T) { p.wait() // must not panic } -func TestRequestPacerCancellationReturnsUnusedReservation(t *testing.T) { - p := &requestPacer{interval: 100 * time.Millisecond} +func TestRequestPacerCancellationDoesNotCollideReservedWaiters(t *testing.T) { + const interval = 80 * time.Millisecond + p := &requestPacer{interval: interval} if err := p.waitContext(context.Background()); err != nil { t.Fatal(err) } p.mu.Lock() - wantNext := p.next + initialNext := p.next p.mu.Unlock() - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond) - defer cancel() - if err := p.waitContext(ctx); err == nil { - t.Fatal("expected paced wait cancellation") + waitForReservation := func(want time.Time) { + t.Helper() + deadline := time.Now().Add(250 * time.Millisecond) + for time.Now().Before(deadline) { + p.mu.Lock() + got := p.next + p.mu.Unlock() + if got.Equal(want) { + return + } + time.Sleep(time.Millisecond) + } + t.Fatalf("reservation did not reach %v", want) } - p.mu.Lock() - gotNext := p.next - p.mu.Unlock() - if !gotNext.Equal(wantNext) { - t.Fatalf("cancelled reservation left pacing debt: next %v, want %v", gotNext, wantNext) + + ctxB, cancelB := context.WithCancel(context.Background()) + bDone := make(chan error, 1) + go func() { bDone <- p.waitContext(ctxB) }() + waitForReservation(initialNext.Add(interval)) + + cDone := make(chan time.Time, 1) + go func() { + _ = p.waitContext(context.Background()) + cDone <- time.Now() + }() + waitForReservation(initialNext.Add(2 * interval)) + + cancelStarted := time.Now() + cancelB() + select { + case err := <-bDone: + if !errors.Is(err, context.Canceled) { + t.Fatalf("B error = %v, want context.Canceled", err) + } + if elapsed := time.Since(cancelStarted); elapsed > 30*time.Millisecond { + t.Fatalf("B cancellation took %v", elapsed) + } + case <-time.After(50 * time.Millisecond): + t.Fatal("B did not return promptly after cancellation") + } + + dDone := make(chan time.Time, 1) + go func() { + _ = p.waitContext(context.Background()) + dDone <- time.Now() + }() + waitForReservation(initialNext.Add(3 * interval)) + + cAt, dAt := <-cDone, <-dDone + if separation := dAt.Sub(cAt); separation < interval/2 { + t.Fatalf("C and D collided: wake separation %v, want at least %v", separation, interval/2) } } diff --git a/internal/web/api.go b/internal/web/api.go index b2fbc93..de21b7a 100644 --- a/internal/web/api.go +++ b/internal/web/api.go @@ -836,7 +836,7 @@ func (s *Server) handleChampionshipHub(w http.ResponseWriter, r *http.Request) { return } if mode == sourceLocal { - markLocalResponse(w, false) + markDataResponse(w, "none", "limited") writeJSON(w, resp) return } @@ -959,6 +959,9 @@ func fetchSeasonRaces(client *api.OpenF1Client, year int) (races []meetingRace, } } if raceKey == 0 { + if isKnownNonChampionshipMeeting(m, sessions) { + return meetingRace{}, false + } failed.Store(true) // The meeting list does not identify non-championship events. Skipping // a meeting without a Race may be expected (testing), but the aggregate @@ -975,6 +978,29 @@ func fetchSeasonRaces(client *api.OpenF1Client, year int) (races []meetingRace, return races, failed.Load(), nil } +func isKnownNonChampionshipMeeting(meeting models.Meeting, sessions []models.Session) bool { + if hasTestingToken(meeting.MeetingName + " " + meeting.MeetingOfficialName) { + return true + } + for _, session := range sessions { + if hasTestingToken(session.SessionName + " " + session.SessionType) { + return true + } + } + return false +} + +func hasTestingToken(value string) bool { + for _, token := range strings.FieldsFunc(strings.ToLower(value), func(r rune) bool { + return (r < 'a' || r > 'z') && (r < '0' || r > '9') + }) { + if token == "test" || token == "tests" || token == "testing" { + return true + } + } + return false +} + // aggregateChampionshipHub is the pure aggregation core (no network) so it can be // unit-tested with synthetic data. races must be ordered ascending by date and // contain only GP meetings (those with a Race session). @@ -1415,9 +1441,11 @@ func (s *Server) handleStrategy(w http.ResponseWriter, r *http.Request) { return } - // Non-race sessions have no stints. + // Empty strategy data may mean a non-race session or a race still settling. if len(stints) == 0 { - markOpenF1AggregateResponse(w, client, driversErr != nil || rcErr != nil) + // Without session-type evidence, an empty primary strategy dataset is + // not enough to prove "not applicable" (it may still be settling). + markOpenF1Availability(w, client, "limited") writeJSON(w, map[string]any{"note": "Not applicable", "drivers": []any{}}) return } diff --git a/internal/web/championship_hub_test.go b/internal/web/championship_hub_test.go index cb4a23e..05d63fd 100644 --- a/internal/web/championship_hub_test.go +++ b/internal/web/championship_hub_test.go @@ -39,7 +39,7 @@ func championshipTestUpstream(t *testing.T, driversOK, meetingHasRace bool) *htt } _, _ = w.Write([]byte(`[{"driver_number":1,"name_acronym":"VER","full_name":"Max Verstappen","team_name":"Red Bull","team_colour":"3671c6"}]`)) case "/v1/meetings": - _, _ = w.Write([]byte(`[{"meeting_key":1,"meeting_name":"Test GP"}]`)) + _, _ = w.Write([]byte(`[{"meeting_key":1,"meeting_name":"Mystery Grand Prix"}]`)) case "/v1/session_result": _, _ = w.Write([]byte(`[{"driver_number":1,"position":1,"points":25}]`)) case "/v1/starting_grid": @@ -86,6 +86,51 @@ func TestFetchSeasonRacesMeetingWithoutRaceIsIncomplete(t *testing.T) { } } +func TestFetchSeasonRacesRecognizedTestingMeetingIsNotIncomplete(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v1/meetings": + _, _ = w.Write([]byte(`[{"meeting_key":1253,"meeting_name":"Pre-Season Testing"}]`)) + case "/v1/sessions": + _, _ = w.Write([]byte(`[{"session_key":1,"meeting_key":1253,"session_name":"Day 1","session_type":"Testing"}]`)) + default: + http.NotFound(w, r) + } + })) + defer upstream.Close() + client := api.NewOpenF1Client(upstream.URL, 2*time.Second) + defer client.Close() + + races, incomplete, err := fetchSeasonRaces(client.Scoped(), time.Now().Year()) + if err != nil { + t.Fatal(err) + } + if incomplete || len(races) != 0 { + t.Fatalf("recognized testing meeting = races %d, incomplete %v", len(races), incomplete) + } +} + +func TestKnownNonChampionshipMeetingRequiresTestingToken(t *testing.T) { + if !isKnownNonChampionshipMeeting(models.Meeting{MeetingName: "Pre-Season Testing"}, nil) { + t.Fatal("pre-season testing was not recognized") + } + if isKnownNonChampionshipMeeting(models.Meeting{MeetingName: "Fastest Grand Prix"}, nil) { + t.Fatal("substring inside a normal word was treated as testing") + } + if !isKnownNonChampionshipMeeting(models.Meeting{MeetingName: "Winter Event"}, []models.Session{{SessionType: "Test"}}) { + t.Fatal("explicit Test session was not recognized") + } +} + +func TestHandleChampionshipHubSourceLocalWithoutAggregateIsLimited(t *testing.T) { + server := NewServer(nil, 0, nil) + recorder := httptest.NewRecorder() + server.handleChampionshipHub(recorder, httptest.NewRequest(http.MethodGet, "/api/v1/championship/hub?year=2026&source=local", nil)) + if recorder.Code != http.StatusOK || recorder.Header().Get(dataSourceHeader) != "none" || recorder.Header().Get(dataFreshnessHeader) != "limited" { + t.Fatalf("empty local championship = %d %q/%q body=%s", recorder.Code, recorder.Header().Get(dataSourceHeader), recorder.Header().Get(dataFreshnessHeader), recorder.Body.String()) + } +} + func raceResult(num, pos int, pts float64) models.SessionResult { return models.SessionResult{DriverNumber: num, Position: pos, Points: pts} } diff --git a/internal/web/component_freshness_test.go b/internal/web/component_freshness_test.go index 6104e99..f6818b3 100644 --- a/internal/web/component_freshness_test.go +++ b/internal/web/component_freshness_test.go @@ -69,6 +69,19 @@ func TestStrategyOptionalComponentFailureReportsPartial(t *testing.T) { assertAvailabilityHeaders(t, recorder, "openf1", "partial") } +func TestStrategyEmptyPrimaryDataReportsLimited(t *testing.T) { + server := componentTestServer(t, map[string]string{ + "/v1/stints": `[]`, + "/v1/pit": `[]`, + "/v1/session_result": `[{"driver_number":1,"position":1,"number_of_laps":10}]`, + "/v1/drivers": `[{"driver_number":1,"full_name":"Max Verstappen","team_name":"Red Bull","team_colour":"3671c6"}]`, + "/v1/race_control": `[]`, + }, nil) + recorder := httptest.NewRecorder() + server.handleStrategy(recorder, httptest.NewRequest(http.MethodGet, "/api/v1/strategy?session_key=99", nil)) + assertAvailabilityHeaders(t, recorder, "openf1", "limited") +} + func TestLapsComparisonDoesNotLabelMissingComponentsFresh(t *testing.T) { tests := []struct { name string diff --git a/internal/web/freshness.go b/internal/web/freshness.go index 523c881..f6d9530 100644 --- a/internal/web/freshness.go +++ b/internal/web/freshness.go @@ -2,8 +2,6 @@ package web import ( "net/http" - - "github.com/AmanTahiliani/box-box/internal/api" ) const ( @@ -11,13 +9,17 @@ const ( dataFreshnessHeader = "X-BoxBox-Data-Freshness" ) +type staleResponseReporter interface { + LastResponseWasStale() bool +} + // markOpenF1Response publishes request-scoped success provenance. Callers must // pass the scoped client used for this response, never Server.client. -func markOpenF1Response(w http.ResponseWriter, client *api.OpenF1Client) { +func markOpenF1Response(w http.ResponseWriter, client staleResponseReporter) { markOpenF1AggregateResponse(w, client, false) } -func markOpenF1AggregateResponse(w http.ResponseWriter, client *api.OpenF1Client, partial bool) { +func markOpenF1AggregateResponse(w http.ResponseWriter, client staleResponseReporter, partial bool) { freshness := "fresh" if partial { freshness = "partial" @@ -25,7 +27,7 @@ func markOpenF1AggregateResponse(w http.ResponseWriter, client *api.OpenF1Client markOpenF1Availability(w, client, freshness) } -func markOpenF1Availability(w http.ResponseWriter, client *api.OpenF1Client, freshness string) { +func markOpenF1Availability(w http.ResponseWriter, client staleResponseReporter, freshness string) { w.Header().Set(dataSourceHeader, "openf1") if client != nil && client.LastResponseWasStale() { w.Header().Set(dataFreshnessHeader, "stale") @@ -51,7 +53,7 @@ func markLocalResponse(w http.ResponseWriter, partial bool) { w.Header().Set(dataFreshnessHeader, "local") } -func markMixedResponse(w http.ResponseWriter, client *api.OpenF1Client, partial bool) { +func markMixedResponse(w http.ResponseWriter, client staleResponseReporter, partial bool) { w.Header().Set(dataSourceHeader, "mixed") if client != nil && client.LastResponseWasStale() { w.Header().Set(dataFreshnessHeader, "stale") diff --git a/internal/web/freshness_test.go b/internal/web/freshness_test.go index 405191f..bb63870 100644 --- a/internal/web/freshness_test.go +++ b/internal/web/freshness_test.go @@ -12,6 +12,20 @@ import ( _ "modernc.org/sqlite" ) +type fakeStaleReporter bool + +func (f fakeStaleReporter) LastResponseWasStale() bool { return bool(f) } + +func TestStaleFreshnessTakesPrecedenceOverPartialAndLimited(t *testing.T) { + for _, fallback := range []string{"partial", "limited"} { + recorder := httptest.NewRecorder() + markOpenF1Availability(recorder, fakeStaleReporter(true), fallback) + if recorder.Header().Get(dataFreshnessHeader) != "stale" { + t.Fatalf("fallback %q overrode stale: %q", fallback, recorder.Header().Get(dataFreshnessHeader)) + } + } +} + func TestOpenF1HandlerReportsFreshThenStaleSuccess(t *testing.T) { year := time.Now().Year() upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/web/replay.go b/internal/web/replay.go index c4d681f..7277013 100644 --- a/internal/web/replay.go +++ b/internal/web/replay.go @@ -113,7 +113,7 @@ func assembleReplayFrames(ctx context.Context, client replayDataClient, sessionK } resp.StartTime = start.Format(time.RFC3339Nano) resp.Frames = snapReplayFrames(series, start, intervalMS) - return resp, err != nil || len(resp.Frames) == 0, nil + return resp, err != nil || len(series) < len(driverNumbers) || len(resp.Frames) == 0, nil } func uniqueDriverNumbers(drivers []models.Driver) []int { diff --git a/internal/web/replay_test.go b/internal/web/replay_test.go index 360b6ce..db24b86 100644 --- a/internal/web/replay_test.go +++ b/internal/web/replay_test.go @@ -107,8 +107,11 @@ func TestAssembleReplayFramesSnapsNearestSamplesAndOmitsEmptyDrivers(t *testing. if err != nil { t.Fatalf("assembleReplayFrames() error = %v", err) } - if incomplete { - t.Fatal("complete driver series reported incomplete") + if !incomplete { + t.Fatal("empty entrant location series was labelled complete") + } + if got := replayResponseFreshness(resp, incomplete); got != "partial" { + t.Fatalf("empty entrant freshness = %q", got) } if resp.SessionKey != 99 || resp.Interval != 5000 { t.Fatalf("response metadata = %+v", resp)