influxdata / influxdata/influxdb

opentsdb: `Service.Close()` does not wait for `handleConn`, causing a data race and a goroutine leak at shutdown

Open
#27,544 0 comments 0 reactions 0 assignees View on GitHub
1.x team/edge
Dominant language
Rust
Stars
31.7k
Forks
3.7k
Avg merge
13h 37m
Merged PRs (30d)
8

Description

**Component:** `services/opentsdb`
**Severity:** Medium — data race (undefined behavior) plus a permanent goroutine/fd leak at shutdown. Surfaces as an intermittent `-race` CI failure.
**Status:** Pre-existing. Not introduced by the TLS work on `gw/clientCert`; reproduced from `master-1.x` code by inspection (see *Provenance*).

## Summary

`Service.serve()` dispatches each accepted connection with a bare `go s.handleConn(conn)` that is **not tracked by `s.wg`**. `Service.Close()` waits on `s.wg` and then mutates `Service` fields, so it can return — and write — while `handleConn` goroutines are still running and reading those same fields.

The same untracked dispatch causes a second, independent defect: a connection accepted during shutdown parks forever on an unguarded channel send, leaking the goroutine and its socket.

## Observed failure

Intermittent under `-race` in CI. Not reproducible locally (see *Reproduction*).

```
=== FAIL: services/opentsdb TestService_TLSUsage (unknown)
==================
WARNING: DATA RACE
Read at 0x00c0002d4020 by goroutine 55:
github.com/influxdata/influxdb/services/opentsdb.(*Service).serve.gowrap1()
services/opentsdb/service.go:373 +0x46

Previous write at 0x00c0002d4020 by goroutine 24:
github.com/influxdata/influxdb/services/opentsdb.TestService_ClientCertAuth.deferwrap1()
services/opentsdb/service_test.go:371 +0x2e
runtime.deferreturn()

Goroutine 55 (running) created at:
github.com/influxdata/influxdb/services/opentsdb.(*Service).serve()
services/opentsdb/service.go:373 +0x2d3
github.com/influxdata/influxdb/services/opentsdb.(*Service).Open.func2()
services/opentsdb/service.go:192 +0x7d

Goroutine 24 (finished) created at:
testing.(*T).Run()
==================
```

Reading the trace:

- `service.go:373` is `go s.handleConn(conn)`, so **goroutine 55 is a `handleConn` goroutine**, and it is still **running**.
- `service_test.go:371` is `defer s.Close()` in `TestService_ClientCertAuth`, and goroutine 24 is **finished** — its `Close()` has already run and written.
- Nothing made `Close()` wait for goroutine 55.

**The reported test name is misleading.** Both accesses belong to `TestService_ClientCertAuth`'s own lifecycle. The race is merely *detected* while a later test happens to be running, so it is attributed to whichever test that is (`TestService_TLSUsage` here). Do not investigate the named test.

## Root cause

`Service.wg` tracks three goroutines:

| Goroutine | Tracked | Site |
|---|---|---|
| `processBatches` | yes | `service.go:157-158` |
| `serve` | yes | `service.go:191-192` |
| `serveHTTP` | yes | `service.go:191,193` |
| **`handleConn`** | **no** | **`service.go:373`** |

```go
// service.go:373 — no s.wg.Add
go s.handleConn(conn)
```

`Close()` therefore waits only for the first three, and then writes:

```go
s.wg.Wait() // service.go:233 — does not cover handleConn

s.mu.Lock()
s.done = nil // service.go:236 — the write in the trace
s.mu.Unlock()
```

`Close()` also writes `s.tlsManager = nil` (`service.go:218`). Any `handleConn` still in flight is concurrently reading `Service` fields, giving the race. `s.done = nil` is taken under `s.mu`, but `handleConn` does not hold `s.mu`, so the mutex buys nothing here.

## Two further defects, same cause

**1. Goroutine + fd leak at shutdown (`service.go:397`).** `handleConn` hands HTTP connections off with an unguarded blocking send:

```go
if err == nil {
atomic.AddInt64(&s.stats.HTTPConnectionsHandled, 1)
s.httpln.ch <- conn // no select on httpln.done
return
}
```

`chanListener.Close()` (`handler.go:177`) closes `ln.done`, not `ln.ch`. Once closed, `chanListener.Accept()` returns `errClosed`, `http.Serve` stops, and **nothing ever drains `ln.ch`** — so a connection arriving during shutdown parks that `handleConn` forever, holding its `net.Conn`. Not a panic (`ch` is never closed), but a permanent leak.

**This is why the race cannot be fixed by simply adding `s.wg.Add(1)` around `go s.handleConn(conn)`:** that converts the leak into a **`Close()` deadlock**, because `wg.Wait()` would then block on the parked send. Both must be fixed together.

**2. `Close()` error paths skip `wg.Wait()` (`service.go:211`, `:214`, `:220`).**

```go
if err := s.ln.Close(); err != nil {
return false, err // → Close() returns without ever calling s.wg.Wait()
}
```

Any listener-close error leaks `serve`, `serveHTTP`, and `processBatches`. `s.done` is already closed at that point (`service.go:207`), so they would exit — but nothing waits for them, and `s.done = nil` is skipped, leaving the service in a half-closed state that `closed()` reports as open. Note `TestService_ClientCertAuth`'s `defer s.Close()` discards this error, so a test would never notice.

**3. `handleConn` calls `s.wg.Add(1)` at `service.go:402`** (around `handleTelnetConn`) — an `Add` that can start while `Close()`'s `Wait` is already in progress, which `sync.WaitGroup` documents as misuse.

## Reproduction

Intermittent; **not reproduced locally** despite:

- `go test -race ./services/opentsdb/ -count=60 -run TestService_ClientCertAuth` — clean
- `go test -race ./services/opentsdb/ -count=10` (full package) — clean
- `go test -race ./services/opentsdb/ -count=20 -run 'TestService_ClientCertAuth|TestService_TLSUsage'` — clean

A goroutine probe run after `TestService_ClientCertAuth` (500 ms settle, `runtime.Stack(all)`) found **zero** lingering `serve`/`handleConn`/`serveHTTP`/`handleTelnetConn` goroutines. So the window between `Close()`'s `wg.Wait()` returning and the last `handleConn` finishing is narrow, and a loaded CI machine widens it. Reproducing reliably will likely need a stress harness that opens a connection and calls `Close()` concurrently, rather than `-count`.

## Impact

- **CI:** intermittent `-race` failures in `services/opentsdb`, attributed to arbitrary tests.
- **Production:** a data race is undefined behavior. In practice the racing accesses are pointer-sized fields, so on amd64 the likely real-world consequence is the **goroutine/fd leak** rather than corruption. `Service.Close()` runs at shutdown and on `influxd` config reload paths that recreate services, so a busy endpoint can leak a goroutine and a socket per shutdown.

## Suggested fix

Handle the dispatch and the handoff together:

1. Track `handleConn` in `s.wg` (`s.wg.Add(1)` before the `go`, `defer s.wg.Done()` inside), and drop the inner `s.wg.Add(1)`/`Done()` at `service.go:402`.
2. Make the handoff abortable so (1) cannot deadlock `Close()`:
```go
select {
case s.httpln.ch <- conn:
case <-s.httpln.done: // or <-s.done
conn.Close()
return
}
```
3. Restructure `Close()` so `s.wg.Wait()` and the `s.done = nil` reset always run, collecting listener-close errors (e.g. `errors.Join`) instead of returning early.

`chanListener.done` is currently unexported and only read inside `Accept`; step 2 needs a small accessor or to select on `s.done` instead.

## Workaround

None needed for correctness of the feature under test. To reduce CI flakiness without touching production code, `TestService_ClientCertAuth` could close its client connections (`Transport.CloseIdleConnections()`) before `defer s.Close()` runs, which narrows the window. This hides the symptom and not the bug, and is not recommended as a substitute for the fix.

## Provenance

Not a regression from the TLS work on `gw/clientCert` (`f77fdde83b`). `git diff master-1.x...HEAD -- services/opentsdb/service.go` touches only lines 50, 94, 107, 151 and 239 — struct fields, `NewService`'s signature, `Open`'s TLS manager options. `serve()`, `handleConn()` and `Close()` are unmodified, and `master-1.x` contains the same `go s.handleConn(conn)` with no `wg.Add` and the same early-return paths in `Close()`.

The branch added `TestService_TLSUsage` and `TestConfig_IgnoreSanityChecks`, which run after `TestService_ClientCertAuth` and lengthen the package run. That gives the detector more opportunity to observe the existing window — the branch made a latent bug visible rather than creating it.

Contributor guide

Open the contributing guide

Research direction

Start in services/opentsdb/service.go, especially serve, handleConn, and Close, then inspect chanListener.Close in handler.go. Run the listed services/opentsdb race tests and add a focused shutdown stress test covering concurrent connections and Close. Done means shutdown waits for connection handlers, aborts blocked handoffs, handles listener-close errors, and no race or goroutine leak remains.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, networking, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.