hanahmily opened a new pull request, #1128:
URL: https://github.com/apache/skywalking-banyandb/pull/1128
### Fix apache/skywalking#13874 — bluge index `analysisWorker` pool not
released on segment rotation
#### Why the bug exists
Standalone BanyanDB deployments leaked ~76 goroutines per UTC midnight:
bluge index `analysisWorker` pools (sized from `GOMAXPROCS`, ~54 goroutines per
writer plus orchestration) stayed alive on segments outside the active write
window. The existing reclaim path (`closeIdleSegments` → `DecRef` →
`performCleanup` → `seriesIndex.Close` → `store.Close` → `writer.Close`) was
correctly implemented but never reached, due to two layered defects:
1. **`segmentIdleTimeout` defaulted to `0`** in non-staged groups
(measure/stream/trace `metadata.go`). The 10-minute reclaim ticker in
`banyand/internal/storage/rotation.go:59` is gated on `>= 1s`, so it never
started in standalone mode.
2. **`incRef` unconditionally refreshed `lastAccessed`**. The rotation tick
handler scans every segment via `segmentController.segments(true)` → `incRef`,
so even if the ticker had been armed, `closeIdleSegments` (which compares
`lastAccessed` against the threshold) would never observe a segment as idle.
#### How the fix works
1. Default `segmentIdleTimeout` to `time.Hour` in
`banyand/{measure,stream,trace}/metadata.go`. Staged `Close: true` still
overrides to 5m.
2. Remove `lastAccessed.Store` from `incRef`. Bump it explicitly at the two
real-access entry points: `selectSegments` (read path) and `createSegment`
(write path). Housekeeping iterators (rotation tick scan, `TakeFileSnapshot`)
no longer falsely refresh idleness.
3. Rewrite `closeIdleSegments` to take its own CAS-bumped snapshot of
`sc.lst` instead of going through `segments(false)`. The old code issued an
unconditional `DecRef` per entry to release a non-atomic `Load > 0 then
Add`-style bump; if a concurrent `selectSegments` reopened an entry the
reclaimer hadn't bumped, that `DecRef` would drop the query's only live ref and
trigger `performCleanup` under active use. Tracking bumped state per entry
isolates the reclaimer from in-flight queries.
4. Surface reclaimer state at startup (`Info` when enabled, `Warn` when
`idle_timeout < 1s`) so the disabled-state is observable without pprof.
#### Test plan
- [x] `go test ./banyand/internal/storage/...` (8.9s, all pass)
- [x] `go test -race ./banyand/internal/storage/... -run
"Segment|Rotation|Idle|Reclaim|Collect"` (7.4s, all pass)
- [x] `make pre-push` — lint, format, license-check, tidy all clean
(vuln-check fails only on Go std-lib CVEs in the local toolchain, unrelated)
- [x] Existing `TestCollectWithPartialClosedSegments` and segment-test
idle-reclaim assertions continue to hold
- [ ] An external 48h soak comparable to the issue's repro to confirm
goroutine count stays flat across rotations
#### Notes
- `segments(false)` still has the same `Load > 0 then Add` shape for its
other callers (`remove`, `getExpiredSegmentsTimeRange`,
`deleteExpiredSegments`, `collect`). Those have the same latent TOCTOU race but
predate this PR; left as follow-up. Comment in the new `closeIdleSegments`
explains why it bypasses `segments(false)`.
- The previous "always bump `lastAccessed` on incRef" behavior in
`openSegment` (initial construction) is retained — that path runs once per
segment open and is harmless.
---
- [x] If this pull request closes/resolves/fixes an existing issue, replace
the issue number. Fixes apache/skywalking#13874.
- [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]
