Wait for ConcurrentLister workers when the bucket listing fails

[arpitjain099]

Fixes #8996.

The reporter's reading of the error path is right, and it still holds on main:

if err = f.bkt.Iter(ctx, "", func(name string) error {
        ...
}); err != nil {
        return nil, err          // <- metaChan not closed, eg.Wait() not called
}
close(metaChan)
if err := eg.Wait(); err != nil {

BaseFetcher.fetchMetadata closes the channel it passed in as soon as this returns:

defer close(activeBlocksCh)
partialBlocks, err = f.blockIDsLister.GetActiveAndPartialBlockIDs(ctx, activeBlocksCh)

so a worker still sitting in its select can send on a closed channel, which is the reported stack at fetcher.go:294.

This closes metaChan and waits on every path, with the Iter error taking precedence when both fail. The close is worth having for its own sake too: cancelling the group context does not break a worker out of a metaChan receive, so a worker parked there would never be released.

On the test

I first tried to assert on the panic itself and could not make it fire reliably, which is worth explaining rather than hiding. Once the group context is cancelled, a parked worker has both <-gCtx.Done() and the send ready, and it usually takes the Done branch; losing that select to the closed channel is exactly the intermittency behind "329 restarts in four days" while another pod ran six days clean. A test built on it would be flaky in whichever direction the scheduler happened to go.

So TestConcurrentLister_WaitsForWorkersOnIterError pins the invariant underneath instead: the lister must not return while a worker is still running. It holds one worker inside Exists on a gate, fails another block to cancel the group, and asserts the call has not returned while the held worker is in flight. That is deterministic, and it fails on main for the right reason:

--- FAIL: TestConcurrentLister_WaitsForWorkersOnIterError
    GetActiveAndPartialBlockIDs returned while a worker was still running;
    the caller closes the activeBlocks channel at this point (#8996)

I also tried a goroutine-leak assertion, which does not catch this: in that configuration the workers are parked in the select rather than on the metaChan receive, so they all exit via Done and nothing leaks. Mentioning it in case it looks like an obvious thing to have used.

Checks

go build, go vet ./pkg/block/ and gofmt are clean, and the new test passes with -race.

For the package suite I compared like for like, since this checkout has no object-storage credentials: 20 failures before the change and the same 20 after, with the failure sets identical once the timings are stripped. They are all environment (insufficient s3 test configuration information, googleapi: Error 400: Unknown project id), not this change.

I have not reproduced the original panic against a real GCS bucket, so the confirmation that Bucket Web stops restarting is still worth having from the reporter.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.54%. Comparing base (51afa7e) to head (9725706).

Files with missing lines	Patch %	Lines
pkg/block/fetcher.go	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8997   +/-   ##
=======================================
  Coverage   64.53%   64.54%           
=======================================
  Files         289      289           
  Lines       37366    37367    +1     
=======================================
+ Hits        24113    24117    +4     
+ Misses      11156    11155    -1     
+ Partials     2097     2095    -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.