Bug: SpanExporter shutdown error not propagated from BatchSpanProcessor.Shutdown

[sakshamngpl]

Problematic Code Location

https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/trace/batch_span_processor.go#L143-L166

Description

I've identified a shadowing bug in the BatchSpanProcessor.Shutdown function that prevents errors returned by the SpanExporter.Shutdown method from being properly propagated to the caller.

Here's the relevant part of the code:

func (bsp *batchSpanProcessor) Shutdown(ctx context.Context) error {
	var err error // (1) Outer 'err' variable declared here
	bsp.stopOnce.Do(func() {
		bsp.stopped.Store(true)
		wait := make(chan struct{})
		go func() {
			close(bsp.stopCh)
			bsp.stopWait.Wait()
			if bsp.e != nil {
				// Problematic line: 'err' is redeclared here using ':=',
				// creating a new local variable.
				if err := bsp.e.Shutdown(ctx); err != nil { // (2) Inner 'err' shadows outer 'err'
					otel.Handle(err)
				}
			}
			close(wait)
		}()
		select {
		case <-wait:
		case <-ctx.Done():
			err = ctx.Err() // (3) Only this line sets the outer 'err'
		}
	})
	return err // (4) Returns the outer 'err'
}

Detailed Explanation:

1. An err variable (var err error) is declared at the top of the Shutdown function's scope (marked as (1) in the code comments above).
2. Inside the go routine, specifically within the if bsp.e != nil block, the line if err := bsp.e.Shutdown(ctx); err != nil utilizes the short variable declaration operator :=.
3. This := creates a new, local err variable that is strictly scoped to that inner if block (marked as (2)).
4. Consequently, any error returned by bsp.e.Shutdown(ctx) is assigned to this inner err variable. While otel.Handle(err) is correctly called with this inner error, it is never assigned to the err variable in the outer Shutdown function's scope.
5. As a result, the BatchSpanProcessor.Shutdown method will only return an error if the provided context.Context is cancelled (ctx.Done(), marked as (3)). Errors originating from the SpanExporter.Shutdown call itself are silently shadowed (except for being handled by otel.Handle) and are not propagated back to the caller of BatchSpanProcessor.Shutdown.

---

Impact

This bug leads to silent failures during exporter shutdown. Callers of BatchSpanProcessor.Shutdown will not be informed if the underlying exporter fails to shut down gracefully. This can mask critical issues during application termination, potentially leading to unhandled resource leaks or data loss.

---

Proposed Simple Solution

The core of the problem is that the := operator inside the if block declares a new, local err variable, effectively hiding the err variable from the function's main scope. To fix this, we simply need to assign any error from the exporter's shutdown directly to the err variable that's already declared at the beginning of the Shutdown function.

Here's the suggested change for the problematic section:

// Inside the go routine, within the if bsp.e != nil block:
if e := bsp.e.Shutdown(ctx); e != nil {
    otel.Handle(e) // Still handle the error globally
    err = e        // Assign the exporter error to the outer 'err'
}

This change ensures that if bsp.e.Shutdown(ctx) returns an error, it's correctly captured by the err variable that the Shutdown function ultimately returns.

[dmathieu]

Do you want to open a PR with the fix and a test?

[seh]

Your proposed solution has two potential assignments to the "err" variable racing, as nothing precludes the assignment within the spawned goroutine from occurring at the same time as the context gets canceled and becomes "done" in the Shutdown caller's goroutine.