Copilot commented on code in PR #1146:
URL:
https://github.com/apache/skywalking-banyandb/pull/1146#discussion_r3334953836
##########
banyand/internal/storage/segment.go:
##########
@@ -440,10 +566,16 @@ func (sc *segmentController[T, O])
selectSegments(timeRange timestamp.TimeRange)
break
}
if s.Overlapping(timeRange) {
- if err = s.incRef(ctx); err != nil {
- return nil, err
+ if reopenClosed {
+ // Real read: reopen if closed and mark as
accessed.
+ if err = s.incRef(ctx); err != nil {
+ return nil, err
+ }
+ s.lastAccessed.Store(now)
+ } else if atomic.LoadInt32(&s.refCount) > 0 {
+ // Stats peek: pin only if already open, never
reopen.
+ atomic.AddInt32(&s.refCount, 1)
}
Review Comment:
When reopenClosed=false, this branch pins an already-open segment via
`LoadInt32` + `AddInt32`. This reintroduces the TOCTOU race described in
`segment.incRef`/`closeIdleSegments`: a concurrent `DecRef` can drop refCount
to 0 and run `performCleanup` (setting `s.index=nil`), and then this `AddInt32`
can resurrect refCount back >0. That can leave `refCount>0` with a cleaned-up
segment, causing future `incRef` calls to skip `initialize` and potentially hit
the same nil-index panic this PR is addressing. Use a CAS loop to bump refCount
only if it remains >0, without reopening or refreshing the idle timer.
##########
CHANGES.md:
##########
@@ -58,6 +58,7 @@ Release Notes.
- Add end-to-end observability for liaison internal queue pipelines with
per-topic metrics for queue_sub and queue_pub, along with Grafana panels and
troubleshooting docs.
- Introduce measure migration tool.
- Support displaying a measure's indexed tags in the dump tool, resolved per
part so peak memory is bounded by the part rather than a segment-wide series
map.
+* Snapshot/backup and data inspection no longer reopen idle-closed segments,
avoiding cold-segment nil-index panics and index lock-file churn.
Review Comment:
CHANGES.md uses `-` list markers in this section, but this entry was added
with `*`, which breaks list formatting/consistency. Consider keeping the same
bullet marker/indentation as the surrounding items so release notes render as a
single list.
--
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]
