fix(aggregator): drain resultsCh on early return and stop polling a closed errCh

[hammadmajid]

Description

AggregateChannel returned on three paths with resultsCh still open and undrained, permanently wedging its producer.

Closes #15

Type of Change

• Bug fix (non-breaking change fixing an issue)
• New feature (non-breaking change adding functionality)
• Performance improvement
• Refactoring or code cleanup
• Documentation update
• CI/CD or build workflow change

Key Changes

Drain on every early return. The pipeline's workers block on resultsCh <- res whose only escape is ctx.Done(). Once the max(32, NumCPU*4) buffer filled: every worker wedged, the dispatcher wedged on tasksCh <- task, wg.Wait() never returned, and neither channel was ever closed — so a later range by the same caller hung too. That is NumCPU+2 leaked goroutines plus a pinned RepositoryReader. AggregateChannel neither cancelled nor drained, and does not own a CancelFunc. Now a named-result defer drains resultsCh whenever the function exits with an error or a cancelled context. The drain is skipped when resultsCh has already been nilled, because ranging a nil channel blocks forever; the termination argument is written out in a comment.

Disable the exhausted errCh arm. A receive on a closed channel is permanently ready, so ok == false fell through and re-entered the select forever. Measured pre-fix: 214,261 select passes over ~100 ms of streaming. Now errCh = nil; continue. The dead if errCh != nil guard that was already in the file was residue of this intended fix; it finally means something.

No more unwritten ordering contract. The post-close error check was a non-blocking default: peek, so it only worked if the producer published its terminal error strictly before closing resultsCh. Nothing documented or enforced that, and this is exported API taking arbitrary channels. Now resultsCh close sets resultsCh = nil, the loop runs while resultsCh != nil || errCh != nil, and finalization happens only once both are closed — an error published after resultsCh closes is surfaced instead of swallowed.

Contracts documented on AggregateChannel and AggregateStream: the producer owns and must close both channels and may publish its terminal error in either order; the caller must pass a context the producer also observes, since the drain only terminates when the producer closes.

Verification & Testing

• Ran go test -v -count=1 ./...
• Ran go test -race -shuffle=on -count=1 ./...
• Ran go vet ./...
• Added or updated unit/integration tests
• Tested manually against sample Git repository histories

go build ./...                                              clean
go vet ./internal/aggregator/                               clean
go test -race -shuffle=on -count=1 ./internal/aggregator/   ok (4 runs for shuffle stability)

Mutation: restored the pre-fix aggregator.go and re-ran — all four new tests fail there (6 leaked goroutines and an undrained channel in both leak subtests, 214,261 select passes in the spin test, error swallowed in the late-error test), then restored the fixed file byte-identically.

TestAggregator_AggregateChannel_ReleasesProducerOnEarlyReturn drives a real search.Pipeline (256 in-memory blobs, deliberately above the result buffer) through both early-return paths and polls runtime.NumGoroutine back to baseline with a 5 s deadline. The existing aggregator tests only ever fed synthetic channels, so the pipeline integration had never been leak-checked. A runWithin helper bounds every call so a blocking regression fails instead of hanging the suite.

Note

Stacked on #22.

API change for reviewers: AggregateChannel now finalizes only when both channels are closed, so producers must close errCh too. search.Pipeline already does on every exit path, and there are no non-test callers (cmd/grg uses the slice-form Aggregate).

Checklist

• gofmt clean
• Every new test mutation-checked: reverting the fix makes it fail
• This layer builds and passes the full race suite on its own, not just at the top of the stack