mrproliu opened a new pull request, #1217:
URL: https://github.com/apache/skywalking-banyandb/pull/1217
## Summary
The liaison leaks one gRPC client-stream goroutine on every write-queue part
sync. Over days this accumulates into hundreds of thousands of parked
goroutines, driving GC CPU up several-fold. This PR cancels the `SyncPart`
stream context on every return path and adds a regression test that guards
against it.
## How it was found
A production liaison raised a CPU alert. Investigation on the live node:
- `liaison-0` CPU rose from **0.67 → 3.4 cores** (~5.5×) over 24h, sustained
— while query/write request rates stayed flat (query ~5 ops/s, write ~300
msg/s). So it was **not** load-driven.
- A 20s CPU profile showed **~94% of CPU in `runtime.gcDrain`**
(`scanobject` / `markroot` / `findObject`) — GC, not application work.
- `go_goroutines` on `liaison-0` climbed monotonically at ~**+1,900/hour**
with no plateau: **347,877** goroutines at capture time (`liaison-1` similarly
affected, ~+39k/24h).
- A goroutine profile showed **347,877 goroutines all parked at the same
frame**:
```
google.golang.org/grpc.newClientStreamWithParams.func4
google.golang.org/[email protected]/stream.go:426
```
This is gRPC's per-stream **client-side** cleanup ("reaper") goroutine. GC
must scan every one of those goroutine stacks as a root each cycle, which
explains the GC-bound CPU.
## Root cause
`chunkedSyncClient.SyncStreamingParts` (`banyand/queue/pub/chunked_sync.go`)
opens a `SyncPart` client stream and, in its `defer`, only calls
`stream.CloseSend()` — it never cancels the stream context.
The gRPC reaper spawned by `newClientStreamWithParams` (for streaming RPCs)
exits **only** when the per-call context is cancelled or the `ClientConn` is
closed:
```go
// grpc stream.go
go func() {
select {
case <-cc.ctx.Done():
cs.finish(ErrClientConnClosing)
case <-ctx.Done():
cs.finish(toRPCErr(ctx.Err()))
}
}()
```
`CloseSend()` does **not** release it. It is released on a clean finish only
if the client drains the stream to the server's final status (`io.EOF`) — but
`SyncStreamingParts` returns without reaching `io.EOF` on several paths
(empty-sync early return, stream error, and the normal path which breaks on the
`SyncResult` frame before reading `io.EOF`).
Crucially, the context passed in is the write-queue syncer's
**process-lifetime** context: `measure/stream/trace` syncers call
`SyncStreamingParts(tst.loopCloser.Ctx(), …)`, and `loopCloser` is
`context.WithCancel(context.Background())` (`pkg/run/closer.go`), cancelled
only at shutdown.
So: streaming RPC + never-cancelled context + never drained to EOF ⇒ **one
reaper goroutine leaked per sync, forever**, on both liaisons. The leak rate
tracks the sync cadence rather than request volume, matching the observed
signature (client-side, monotonic, server-side stream counts balanced).
## Fix
Derive a per-call cancellable context for the stream and `defer cancel()`,
so the reaper is released on every return path:
```go
// banyand/queue/pub/chunked_sync.go
streamCtx, cancel := context.WithCancel(ctx)
defer cancel()
stream, err := chunkedClient.SyncPart(streamCtx)
```
Minimal and behavior-preserving: the context stays live for the whole call
(the entire stream lifetime), and `cancel()` runs only after the function
returns. This covers the empty-sync, error, and `SyncResult`-break paths
uniformly.
## Test
Adds `TestSyncStreamingPartsDoesNotLeakReaperGoroutines`
(`banyand/queue/pub/chunked_sync_leak_test.go`) — a standard, deterministic
unit test that runs with `go test`/CI on every commit:
- Reproduces the production trigger: many syncs sharing a single
**never-cancelled** context.
- Counts live goroutines parked in `newClientStreamWithParams` via
`runtime.Stack` (whole-block counting, robust to grpc's closure numbering and
unrelated goroutine noise).
- Asserts the count returns to baseline (no leak). Per-iteration
`result.Success` assertions guarantee the stream path actually executed, so a
clean count is not a vacuous pass.
- Reuses the existing `setupChunkedSyncServer` / `createFileInfo` harness;
no production code is touched by the test.
## Verification
- **With the fix:** `PASS` — `baseline=0 after=0 leaked=0` over 30 syncs
with a never-cancelled context.
- **Reverting the fix:** `FAIL` — `leaked=30` (exactly one reaper per sync),
with the leaked `newClientStreamWithParams.func4` stack printed. Confirms the
test has teeth.
- Existing chunked-sync retry specs still pass; `gofmt` and `go vet` clean.
## Notes
- The reaper mechanism is unchanged between grpc `v1.80.0` (deployed) and
`v1.82.0` (current `go.mod`) — the leak is a context-management bug on the
BanyanDB side, not a grpc version issue.
- Both liaison nodes were affected; a rolling restart clears the accumulated
goroutines as a stop-gap, but this fix is required to stop recurrence.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace
the issue number. Fixes apache/skywalking#<issue number>.
- [x] Update the [`CHANGES`
log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
