From f73247f5810ce22d75d42c97b424a881f33cefa9 Mon Sep 17 00:00:00 2001 From: TheMythologist Date: Thu, 9 Jul 2026 14:39:33 -0700 Subject: [PATCH] fix(optimiser): retry and parallelise module data fetches Module timetable data was fetched from api.nusmods.com one module at a time with a single 10s client timeout and no retries, so one slow fetch aborted the entire solve. Production logs showed a solve fail on PC5102.json with "context deadline exceeded (while awaiting headers)". nginx origin logs proved the origin was healthy: the same PC5102.json was served as a fast 200 (703 bytes) roughly 11s after the optimiser dispatched it, ~1s after the client had already timed out. The stall was in the optimiser -> Cloudflare transit hop during a burst of concurrent optimiser requests, not origin compute and not rate-limiting (a limit would return a status code, with headers). Changes: - Retry transient failures (network/timeout/5xx) up to 3 attempts with a 300ms backoff; 4xx (e.g. unknown module 404) is treated as permanent and not retried. - Fetch and process modules concurrently instead of sequentially. Over HTTP/2 these multiplex over a single connection, so wall-clock is bounded by the slowest module rather than their sum. - Lower the per-attempt timeout from 10s to 4s so a stalled connection is abandoned quickly and a fresh one is retried, rather than burning the whole budget on one attempt. Co-Authored-By: Claude Opus 4.8 (1M context) --- website/api/optimiser/_client/client.go | 49 +++++- website/api/optimiser/_constants/constants.go | 17 ++ website/api/optimiser/_modules/modules.go | 153 ++++++++++++------ 3 files changed, 163 insertions(+), 56 deletions(-) diff --git a/website/api/optimiser/_client/client.go b/website/api/optimiser/_client/client.go index aeca5f4030..68dc68cb97 100644 --- a/website/api/optimiser/_client/client.go +++ b/website/api/optimiser/_client/client.go @@ -2,6 +2,7 @@ package client import ( + "errors" "fmt" "io" "net/http" @@ -10,10 +11,52 @@ import ( constants "github.com/nusmodifications/nusmods/website/api/optimiser/_constants" ) -var httpClient = &http.Client{Timeout: 10 * time.Second} +var httpClient = &http.Client{Timeout: constants.HTTPRequestTimeout} -// HTTP request to get Module data +// badStatusError signals a non-200 response. It carries the status code so the +// retry logic can decide whether the failure is worth retrying (5xx) or is a +// permanent client error such as an unknown module (4xx). +type badStatusError struct { + module string + statusCode int +} + +func (e *badStatusError) Error() string { + return fmt.Sprintf("failed to fetch module %s: status %d", e.module, e.statusCode) +} + +// GetModuleData fetches a module's JSON data, retrying transient failures. +// +// Network errors and timeouts (the origin being briefly slow to respond) and +// 5xx responses are retried with a fixed backoff. A 4xx response is treated as +// permanent (e.g. an unknown module returns 404) and is not retried. func GetModuleData(acadYear string, module string) ([]byte, error) { + var lastErr error + for attempt := 1; attempt <= constants.HTTPMaxAttempts; attempt++ { + body, err := fetchModuleData(acadYear, module) + if err == nil { + return body, nil + } + lastErr = err + + // Don't retry permanent client errors (4xx) — retrying won't help. + var statusErr *badStatusError + if errors.As(err, &statusErr) && + statusErr.statusCode >= http.StatusBadRequest && + statusErr.statusCode < http.StatusInternalServerError { + return nil, err + } + + // Don't sleep after the final attempt. + if attempt < constants.HTTPMaxAttempts { + time.Sleep(constants.HTTPRetryBackoff) + } + } + return nil, fmt.Errorf("giving up after %d attempts: %w", constants.HTTPMaxAttempts, lastErr) +} + +// fetchModuleData performs a single HTTP request for a module's JSON data. +func fetchModuleData(acadYear string, module string) ([]byte, error) { url := fmt.Sprintf(constants.ModulesURL, acadYear, module) res, err := httpClient.Get(url) if err != nil { @@ -22,7 +65,7 @@ func GetModuleData(acadYear string, module string) ([]byte, error) { defer res.Body.Close() if res.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to fetch module %s: status %d", module, res.StatusCode) + return nil, &badStatusError{module: module, statusCode: res.StatusCode} } body, err := io.ReadAll(res.Body) diff --git a/website/api/optimiser/_constants/constants.go b/website/api/optimiser/_constants/constants.go index bfb9052066..d567bc3621 100644 --- a/website/api/optimiser/_constants/constants.go +++ b/website/api/optimiser/_constants/constants.go @@ -2,10 +2,27 @@ package constants import ( _ "embed" + "time" models "github.com/nusmodifications/nusmods/website/api/optimiser/_models" ) +// HTTP client settings for fetching module data from the NUSMods API. +const ( + // HTTPRequestTimeout bounds a single request (connect + awaiting headers + + // reading the body), applied per attempt rather than across all retries. + // Observed failures are connection/transit stalls on the way to the origin + // (the origin itself serves in milliseconds), not slow origin compute, so + // this is kept short to fail fast and let a retry try a fresh connection + // instead of blocking the whole budget on one stalled attempt. + HTTPRequestTimeout = 4 * time.Second + // HTTPMaxAttempts is the total number of attempts (1 initial + retries) for + // a single module before giving up. + HTTPMaxAttempts = 3 + // HTTPRetryBackoff is the fixed delay between attempts. + HTTPRetryBackoff = 300 * time.Millisecond +) + // Ensure in sync with all E-Venues in NUSMods var EVenues = map[string]struct{}{ "E-Learn_A": {}, diff --git a/website/api/optimiser/_modules/modules.go b/website/api/optimiser/_modules/modules.go index b3bb1a253f..2b37fd0b1b 100644 --- a/website/api/optimiser/_modules/modules.go +++ b/website/api/optimiser/_modules/modules.go @@ -6,6 +6,7 @@ import ( "sort" "strconv" "strings" + "sync" client "github.com/nusmodifications/nusmods/website/api/optimiser/_client" constants "github.com/nusmodifications/nusmods/website/api/optimiser/_constants" @@ -36,72 +37,118 @@ func GetAllModuleSlots( // These are default or backup slots for the partial timetable so that we can display some random slot for unallocated lessons defaultSlots := make(models.ModuleDefaultSlotsMap) - for _, module := range optimiserRequest.Modules { - body, err := client.GetModuleData(optimiserRequest.AcadYear, strings.ToUpper(module)) - if err != nil { - return nil, nil, nil, err - } + // Fetch and process each module concurrently. The modules are independent of + // one another, so a single slow fetch no longer blocks the others; the total + // wall-clock time is bounded by the slowest module rather than their sum. + // Results are written into per-index slots to avoid concurrent map writes, + // then assembled into the maps once all goroutines have finished. + type moduleResult struct { + merged map[models.LessonType]map[models.ClassNo][]models.ModuleSlot + defaults map[models.LessonType][]models.ModuleSlot + err error + } + results := make([]moduleResult, len(optimiserRequest.Modules)) + + var wg sync.WaitGroup + for i, module := range optimiserRequest.Modules { + wg.Add(1) + go func(i int, module string) { + defer wg.Done() + merged, defaults, err := processModule( + optimiserRequest, + module, + venues, + recordingsMap, + freeDaysMap, + ) + results[i] = moduleResult{merged: merged, defaults: defaults, err: err} + }(i, module) + } + wg.Wait() - var moduleData struct { - SemesterData []struct { - Semester int `json:"semester"` - Timetable []models.ModuleSlot `json:"timetable"` - } `json:"semesterData"` - } - err = json.Unmarshal(body, &moduleData) - if err != nil { - return nil, nil, nil, err + for i, module := range optimiserRequest.Modules { + if results[i].err != nil { + return nil, nil, nil, results[i].err } + moduleSlots[module] = results[i].merged + defaultSlots[module] = results[i].defaults + } - // Get the module timetable for the semester - var moduleTimetable []models.ModuleSlot - for _, semester := range moduleData.SemesterData { - if semester.Semester == optimiserRequest.AcadSem { - moduleTimetable = semester.Timetable - break - } - } + return moduleSlots, defaultSlots, recordingsMap, nil +} + +// processModule fetches a single module's data and reduces it to its candidate +// and default slots. It is safe to run concurrently for different modules since +// it only reads the shared venues/recordings/freeDays maps and returns its +// result rather than writing to shared state. +func processModule( + optimiserRequest *models.OptimiserRequest, + module string, + venues map[string]models.Location, + recordingsMap map[string]struct{}, + freeDaysMap map[string]struct{}, +) (map[models.LessonType]map[models.ClassNo][]models.ModuleSlot, map[models.LessonType][]models.ModuleSlot, error) { + body, err := client.GetModuleData(optimiserRequest.AcadYear, strings.ToUpper(module)) + if err != nil { + return nil, nil, err + } - // Parse the weeks - for i := range moduleTimetable { + var moduleData struct { + SemesterData []struct { + Semester int `json:"semester"` + Timetable []models.ModuleSlot `json:"timetable"` + } `json:"semesterData"` + } + err = json.Unmarshal(body, &moduleData) + if err != nil { + return nil, nil, err + } - // Note: if weeks is not a []int, then skip parsing - // Currently we are not handling week conflict for non-[]int weeks - if _, ok := moduleTimetable[i].Weeks.([]any); !ok { - continue - } + // Get the module timetable for the semester + var moduleTimetable []models.ModuleSlot + for _, semester := range moduleData.SemesterData { + if semester.Semester == optimiserRequest.AcadSem { + moduleTimetable = semester.Timetable + break + } + } - moduleTimetable[i].WeeksSet = make(map[int]struct{}) - weeks := moduleTimetable[i].Weeks.([]any) - weeksStrings := make([]string, 0, len(weeks)) + // Parse the weeks + for i := range moduleTimetable { - for _, week := range weeks { - weekFloat, ok := week.(float64) - if !ok { - continue - } - weekInt := int(weekFloat) - moduleTimetable[i].WeeksSet[weekInt] = struct{}{} - weeksStrings = append(weeksStrings, strconv.Itoa(weekInt)) - } - moduleTimetable[i].WeeksString = strings.Join(weeksStrings, ",") + // Note: if weeks is not a []int, then skip parsing + // Currently we are not handling week conflict for non-[]int weeks + if _, ok := moduleTimetable[i].Weeks.([]any); !ok { + continue } - // Store the module slots for the module - moduleSlots[module], defaultSlots[module] = mergeAndFilterModuleSlots( - moduleTimetable, - venues, - module, - recordingsMap, - freeDaysMap, - optimiserRequest.EarliestMin, - optimiserRequest.LatestMin, - ) + moduleTimetable[i].WeeksSet = make(map[int]struct{}) + weeks := moduleTimetable[i].Weeks.([]any) + weeksStrings := make([]string, 0, len(weeks)) + for _, week := range weeks { + weekFloat, ok := week.(float64) + if !ok { + continue + } + weekInt := int(weekFloat) + moduleTimetable[i].WeeksSet[weekInt] = struct{}{} + weeksStrings = append(weeksStrings, strconv.Itoa(weekInt)) + } + moduleTimetable[i].WeeksString = strings.Join(weeksStrings, ",") } - return moduleSlots, defaultSlots, recordingsMap, nil + merged, defaults := mergeAndFilterModuleSlots( + moduleTimetable, + venues, + module, + recordingsMap, + freeDaysMap, + optimiserRequest.EarliestMin, + optimiserRequest.LatestMin, + ) + return merged, defaults, nil } // Gets all venue information from venues.json