influxdata / influxdata/influxdb
opentsdb: `Service.Close()` does not wait for `handleConn`, causing a data race and a goroutine leak at shutdown
- 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
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