Copilot commented on code in PR #1216:
URL:
https://github.com/apache/skywalking-banyandb/pull/1216#discussion_r3566213875
##########
banyand/internal/storage/disk_monitor.go:
##########
@@ -169,15 +173,24 @@ func (dm *DiskMonitor) Stop() {
}
close(dm.stopCh)
- // Wait for any active cleanup to finish
- for dm.isActive.Load() {
- time.Sleep(100 * time.Millisecond)
- }
+ // Wait for the monitor loop (including any in-flight cleanup cycle) to
+ // exit. The loop clears isActive on exit, so we must not poll isActive
+ // here: once stopCh is closed the loop may return without ever running
+ // another cleanup cycle, leaving isActive set and hanging Stop()
forever.
+ <-dm.doneCh
Review Comment:
Stop() now blocks on doneCh, but doneCh is only closed by Start() (disabled
path) or monitorLoop(). If a DiskMonitor is constructed and Stop() is called
before Start(), Stop() will block forever. Consider making Stop() safe when the
monitor loop was never started (e.g., close doneCh or skip waiting when
dm.ticker is nil).
##########
banyand/internal/storage/disk_monitor_test.go:
##########
@@ -244,6 +244,40 @@ func TestDiskMonitor_StartStop(t *testing.T) {
assert.False(t, dm.isActive.Load())
}
+func TestDiskMonitor_StopDoesNotHangWhenActive(t *testing.T) {
+ service := NewMockRetentionService()
+ config := RetentionConfig{
+ HighWatermark: 80.0,
+ LowWatermark: 60.0,
+ CheckInterval: time.Hour, // long interval so no cleanup
cycle races the test
+ Cooldown: time.Millisecond,
+ ForceCleanupEnabled: true,
+ }
+ registry := createMockMetricsRegistry()
+
+ dm := NewDiskMonitor(service, config, registry)
+ dm.Start()
+
+ // Simulate a forced cleanup that is still active when the monitor is
+ // stopped. Previously Stop() polled isActive, which the monitor loop
only
+ // clears from within a cleanup cycle; closing stopCh made the loop exit
+ // without clearing it, hanging Stop() forever.
+ dm.isActive.Store(true)
+
+ stopped := make(chan struct{})
+ go func() {
+ dm.Stop()
+ close(stopped)
+ }()
+
+ select {
+ case <-stopped:
+ case <-time.After(5 * time.Second):
+ t.Fatal("Stop() hung while a forced cleanup was active")
+ }
+ assert.False(t, dm.isActive.Load())
+}
Review Comment:
This regression test covers the "active cleanup" shutdown case, but it
doesn’t cover the new failure mode where Stop() can hang if it’s called before
Start() (since Stop() now waits on doneCh). Adding a small sub-check here would
prevent reintroducing that deadlock.
--
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]
