perf: do api queries in parallel using errgroup

[G-Rath]

This started off as a refactor to replace our bespoke semaphore/channel based implementation for doing osv hydration in parallel with the errgroup package, but then I realized it can be applied to querying of vulns too.

I'd previously not done this because we already fetch in batches of 1000 packages at a time (by default), and in my experience most projects will have less than 1000 per lockfile (even JavaScript ones tend to be be just under the 1000 package mark half the time), but I have seen a few (usually frontend SPAs) that have upwards of 1000 for which this does noticeably make things faster.

[Copilot]

Pull request overview

This PR refactors the parallel API query implementation to use golang.org/x/sync/errgroup instead of a custom semaphore/channel-based approach. The change applies to both vulnerability hydration (FetchAll) and batch querying (Check), improving code maintainability and potentially speeding up processing for projects with many packages.

Key changes:

• Replaced custom semaphore/channel concurrency control with errgroup in FetchAll() and Check()
• Added golang.org/x/sync v0.16.0 dependency
• Modified test to accommodate parallel batch execution using content-length based request differentiation

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pkg/database/api-fetch-all.go	Simplified parallel fetching logic using errgroup with a 200 concurrent operation limit, replacing the previous semaphore/channel implementation
pkg/database/api-check.go	Added parallel batch processing using errgroup to execute multiple batch queries concurrently
pkg/database/api-check_test.go	Updated test to handle non-deterministic request ordering from parallel batch execution by switching from request count to content-length based differentiation
go.mod	Added golang.org/x/sync v0.16.0 dependency
go.sum	Added checksums for golang.org/x/sync v0.16.0 dependency

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[Copilot]

Missing concurrency limit for errgroup

Unlike FetchAll() which sets a limit of 200 concurrent operations with eg.SetLimit(200) (line 12 in api-fetch-all.go), this errgroup has no limit. This could potentially spawn a large number of goroutines if there are many batches, which may overwhelm system resources or the API server.

Suggestion: Consider adding a concurrency limit consistent with the existing pattern:

var eg errgroup.Group
eg.SetLimit(200)  // or another appropriate limit

Suggested change

	var eg errgroup.Group
	var eg errgroup.Group
	eg.SetLimit(200)

[Copilot]

Race condition: closure captures loop variable incorrectly

The closure passed to eg.Go() captures the loop variables i and id by reference, not by value. Since multiple goroutines are created in rapid succession, they will likely all reference the final values of i and id from the loop, causing incorrect behavior and potential race conditions.

This will result in multiple goroutines writing to the same index in the osvs slice, and potentially fetching the same ID multiple times while missing others.

Solution: Capture the loop variables by passing them as parameters to an immediately invoked function, or assign them to local variables before the closure:

for i, id := range ids {
    i, id := i, id  // capture loop variables
    eg.Go(func() error {
        osv, _ := db.Fetch(id)
        osvs[i] = osv
        return nil
    })
}

[G-Rath]

I'm pretty sure this is incorrect since Go 1.22

[Copilot]

Race condition: closure captures loop variable incorrectly

The closure passed to eg.Go() captures the loop variables i and batch by reference, not by value. Since multiple goroutines are created in rapid succession, they will likely all reference the final values of these variables from the loop.

This will cause multiple goroutines to process the same batch and write to the same index in batchResults, while other batches are never processed.

Solution: Capture the loop variables before the closure:

for i, batch := range batches {
    i, batch := i, batch  // capture loop variables
    eg.Go(func() error {
        results, err := db.checkBatch(batch)
        if err != nil {
            return err
        }
        batchResults[i] = results
        return nil
    })
}

[Copilot]

Race condition in test: concurrent writes to requestCount

With parallel batch execution, multiple goroutines may increment requestCount concurrently without synchronization, leading to a race condition. This can cause incorrect request counting and test flakiness.

Solution: Use sync/atomic to safely increment the counter:

var requestCount atomic.Int32

mux.HandleFunc("/querybatch", func(w http.ResponseWriter, r *http.Request) {
    count := requestCount.Add(1)
    
    if count > 2 {
        t.Errorf("unexpected number of requests (%d)", count)
    }
    // ... rest of the handler

And at line 611:

if requestCount.Load() != 2 {

[G-Rath]

I'm fine with this as it should be very low risk (its not been an issue so far)