Copilot commented on code in PR #1012:
URL:
https://github.com/apache/skywalking-banyandb/pull/1012#discussion_r2946430552
##########
banyand/stream/tstable_test.go:
##########
@@ -250,6 +250,22 @@ func Test_tstIter(t *testing.T) {
})
}
+func Test_mustAddMemPart_closeNotifyReleasesMemPart(t *testing.T) {
+ tst := &tsTable{
+ loopCloser: run.NewCloser(1),
+ introductions: make(chan *introduction),
+ }
+
+ mp := generateMemPart()
+ mp.mustInitFromElements(esTS1)
+ require.Greater(t, mp.partMetadata.TotalCount, uint64(0))
+
+ tst.Close()
+ tst.mustAddMemPart(mp)
+
+ require.Equal(t, uint64(0), mp.partMetadata.TotalCount)
Review Comment:
The assertion reads fields from `mp` after `tst.mustAddMemPart(mp)`
potentially decrements the refcount and returns the object to a pool. If
`decRef()` releases to a pool, accessing `mp.partMetadata` after release can be
unsafe/flaky if the object is reused. Consider asserting release via a test
hook (e.g., a “released” flag set during release), refcount visibility, or
another non-pooled observable effect instead of reading mutable struct fields
post-release.
##########
banyand/measure/tstable_test.go:
##########
@@ -297,6 +297,22 @@ var fieldProjections = map[int][]string{
3: {"intField"},
}
+func Test_mustAddMemPart_closeNotifyReleasesMemPart(t *testing.T) {
+ tst := &tsTable{
+ loopCloser: run.NewCloser(1),
+ introductions: make(chan *introduction),
+ }
+
Review Comment:
These tests construct `tsTable` via a partial struct literal, which is
brittle if `tsTable` gains new required fields/invariants (or if
`Close()`/`mustAddMemPart()` starts depending on additional initialization).
Prefer using an existing constructor/test helper for `tsTable` (or introduce
one) to keep the test resilient to internal refactors.
##########
banyand/trace/write_data.go:
##########
@@ -103,6 +105,7 @@ func (s *syncPartContext) Close() error {
s.writers = nil
}
if s.memPart != nil {
+ releaseMemPart(s.memPart)
Review Comment:
`tsTable.mustAddMemPart` uses ref-counting (`decRef()`), while
`syncPartContext.Close()` releases via `releaseMemPart(...)`. If `memPart`
ownership is consistently refcounted, mixing raw “release to pool” and
refcount-based release can cause under/over-release bugs when the acquisition
path changes. Consider using the same release primitive in both places (e.g.,
`decRef()` if that’s the intended ownership model) or add a short comment
clarifying why `syncPartContext` owns an unreferences memPart that must be
released directly.
--
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]
