fix(optimiser): retry and parallelise module data fetches to fix intermittent timeouts#4459
fix(optimiser): retry and parallelise module data fetches to fix intermittent timeouts#4459TheMythologist wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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) | ||
| } |
There was a problem hiding this comment.
When a request contains many modules, this loop starts one goroutine per module and each goroutine can make up to three outbound API attempts. A single oversized optimiser request can now create a burst of simultaneous HTTP calls, which can exhaust local resources or trigger upstream throttling instead of keeping outbound pressure bounded as the old sequential path did.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/api/optimiser/_modules/modules.go
Line: 54-67
Comment:
**Unbounded Request Fanout**
When a request contains many modules, this loop starts one goroutine per module and each goroutine can make up to three outbound API attempts. A single oversized optimiser request can now create a burst of simultaneous HTTP calls, which can exhaust local resources or trigger upstream throttling instead of keeping outbound pressure bounded as the old sequential path did.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
@TheMythologist I think this is a valid point also considering that we are working with not very powerful cpus in Vercel. Perhaps limiting it to some number of goroutines (ex: 4) using a Semaphore?
| 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 |
There was a problem hiding this comment.
When one module has a permanent error such as a 404, the request still waits for every already-started module goroutine to finish before returning that error. Mixed valid and invalid requests now make unnecessary external calls and can wait on unrelated slow retries, whereas the old sequential path stopped at the first failing module.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/api/optimiser/_modules/modules.go
Line: 68-72
Comment:
**Terminal Errors Wait For All Fetches**
When one module has a permanent error such as a 404, the request still waits for every already-started module goroutine to finish before returning that error. Mixed valid and invalid requests now make unnecessary external calls and can wait on unrelated slow retries, whereas the old sequential path stopped at the first failing module.
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4459 +/- ##
==========================================
+ Coverage 54.52% 58.48% +3.96%
==========================================
Files 274 316 +42
Lines 6076 7294 +1218
Branches 1455 1794 +339
==========================================
+ Hits 3313 4266 +953
- Misses 2763 3028 +265 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
| 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() |
There was a problem hiding this comment.
The pull request changes the module processing logic in GetAllModuleSlots from sequential execution to concurrent execution using Go goroutines. However, it does not enforce any limit on the number of concurrent goroutines spawned, nor does it validate or limit the size of the modules array in the incoming OptimiserRequest.
An unauthenticated attacker can submit a request with an arbitrarily large number of modules (e.g., thousands of duplicate or unique module codes). The backend will attempt to spawn a goroutine and initiate an outbound HTTP request to the NUSMods API for each module concurrently.
This unbounded concurrency can quickly exhaust system resources, specifically:
- File Descriptors: Each concurrent HTTP request opens a network socket. If the number of concurrent requests exceeds the operating system's file descriptor limit (typically 1024), the process will fail to open new sockets, causing errors for all concurrent users.
- Memory and CPU: Allocating goroutines, HTTP request/response buffers, and parsing JSON payloads concurrently for thousands of modules will exhaust the memory and CPU of the serverless function, causing Vercel to terminate the execution environment.
- IP Rate Limiting: The sudden burst of thousands of concurrent requests to
api.nusmods.comcan trigger rate-limiting or IP blocks, disrupting the service for legitimate users.
Steps to Reproduce
An attacker can send a POST request to the optimiser endpoint with a payload containing a large number of module codes:
{
"modules": ["CS1010S", "CS1010S", ... (repeated 5000 times) ...],
"recordings": [],
"freeDays": [],
"earliestTime": "0800",
"latestTime": "1800",
"acadYear": "2024-2025",
"acadSem": 1,
"maxConsecutiveHours": 4,
"lunchStart": "1200",
"lunchEnd": "1300"
}This will trigger 5,000 concurrent goroutines and outbound HTTP requests, leading to socket exhaustion or serverless function termination.
Fix with AI
A security vulnerability was found by Hacktron.
File: website/api/optimiser/_modules/modules.go
Lines: 53-68
Severity: high
Vulnerability: Unbounded Concurrency in Module Processing Leads to Denial of Service (DoS) via Resource Exhaustion
Description:
The pull request changes the module processing logic in `GetAllModuleSlots` from sequential execution to concurrent execution using Go goroutines. However, it does not enforce any limit on the number of concurrent goroutines spawned, nor does it validate or limit the size of the `modules` array in the incoming `OptimiserRequest`.
An unauthenticated attacker can submit a request with an arbitrarily large number of modules (e.g., thousands of duplicate or unique module codes). The backend will attempt to spawn a goroutine and initiate an outbound HTTP request to the NUSMods API for each module concurrently.
This unbounded concurrency can quickly exhaust system resources, specifically:
1. **File Descriptors**: Each concurrent HTTP request opens a network socket. If the number of concurrent requests exceeds the operating system's file descriptor limit (typically 1024), the process will fail to open new sockets, causing errors for all concurrent users.
2. **Memory and CPU**: Allocating goroutines, HTTP request/response buffers, and parsing JSON payloads concurrently for thousands of modules will exhaust the memory and CPU of the serverless function, causing Vercel to terminate the execution environment.
3. **IP Rate Limiting**: The sudden burst of thousands of concurrent requests to `api.nusmods.com` can trigger rate-limiting or IP blocks, disrupting the service for legitimate users.
Proof of Concept:
An attacker can send a POST request to the optimiser endpoint with a payload containing a large number of module codes:
```json
{
"modules": ["CS1010S", "CS1010S", ... (repeated 5000 times) ...],
"recordings": [],
"freeDays": [],
"earliestTime": "0800",
"latestTime": "1800",
"acadYear": "2024-2025",
"acadSem": 1,
"maxConsecutiveHours": 4,
"lunchStart": "1200",
"lunchEnd": "1300"
}
```
This will trigger 5,000 concurrent goroutines and outbound HTTP requests, leading to socket exhaustion or serverless function termination.
Affected Code:
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()
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
thejus03
left a comment
thejus03
left a comment
There was a problem hiding this comment.
I don't know if I see the worth in making requests to api.nusmods.com concurrent because it adds unnecessary additional complexity and security issues when the query itself usually completes in milliseconds for all modules.
2 participants
Problem
Some optimiser requests were failing with errors like:
solve failed: Get "https://api.nusmods.com/v2/2026-2027/modules/PC5102.json": context deadline exceeded (Client.Timeout exceeded while awaiting headers)The optimiser fetched each module's timetable data from
api.nusmods.comsequentially, with a single 10s client timeout and no retries. Any one slow fetch aborted the entire solve.Root cause — the origin is healthy; the stall is in transit
Correlating the optimiser's error log (UTC) with the nginx origin access log (SGT, +0800).
14:59 UTC = 22:59 SGT:14:59:00.016→22:59:00"request received"22:59:00PC5101→200(785 B)22:59:13PC5102→200(703 B)14:59:11.795→22:59:11.8"solve failed: PC5102 timeout"The optimiser dispatched
PC5102at ~22:59:01–02, its 10s timeout fired at 22:59:11.8, and the origin didn't servePC5102until 22:59:13 — ~11s after dispatch, and when it did, it was a trivially fast200for a 703-byte file. The origin never struggled; the request simply took ~11s to reach it.Additional context from the log: 22:53–23:03 SGT shows a dense burst of
Go-http-client/2.0requests across dozens of modules (many concurrent optimiser invocations, AY26/27 registration season). This is a load-induced connection/transit stall on the optimiser → Cloudflare hop (likely cold Vercel instances doing fresh TLS handshakes from shared egress IPs during a burst).Ruled out:
awaiting headersmeans no response headers arrived at all. No429/503anywhere in the origin log.PC5102.jsonwas served as a fast200/304hundreds of times that day, including seconds before and after the failure.Changes
4xx(e.g. unknown module404) is treated as permanent and not retried, so bad requests still fail fast.Note
This fix makes the optimiser resilient to the transit stalls. It does not address the underlying infra cause (Vercel egress / Cloudflare connection reuse under burst) — worth a follow-up with Cloudflare logs if timeouts persist after deploy.