fix(optimiser): retry and parallelise module data fetches to fix intermittent timeouts

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)

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":  {},

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