hanahmily opened a new pull request, #1216: URL: https://github.com/apache/skywalking-banyandb/pull/1216
# Fix flaky `Forced TTL Cleanup` goroutine-leak failure (shutdown deadlock in `DiskMonitor.Stop`) ## Symptom The scheduled **Flaky Test** action ([run 29129685572](https://github.com/apache/skywalking-banyandb/actions/runs/29129685572)) failed in the `Forced TTL Cleanup` spec of `TestPropertyIntegrationOther`: ``` Summarizing 1 Failure: [FAIL] Forced TTL Cleanup [AfterEach] should trigger forced cleanup based on disk watermarks --- FAIL: TestPropertyIntegrationOther (206.11s) ``` The `AfterEach` goroutine-leak assertion timed out: ``` [FAILED] Timed out after 30.003s. Expected not to leak 346 goroutines: ... pkg/flow/streaming.(*unaryOperator).run ... ... banyand/measure.(*topNStreamingProcessor[...]).handleError ... ... banyand/internal/storage.(*database[...]).startRotationTask ... ... oklog/run.(*Group).Run ... ``` ## Root cause — this is NOT a TopN / streaming-flow leak The 346 leaked goroutines look like a TopN streaming-pipeline leak, but that is a red herring. Reading the full goroutine dump, **every one of the 20 leaked TopN flows is blocked on `chan receive`, waiting for input** — the source `Transmit` goroutines are parked on `for elem := range current.Out()`, which only happens when `close(src)` (i.e. `processor.Close()`) was never called. In a real cascade/back-pressure deadlock we would instead see goroutines blocked on `chan send`. So the flows were never told to shut down at all. Why not? The dump contains the main shutdown goroutine: ``` goroutine 19816 [sleep] time.Sleep(...) at storage/disk_monitor.go:174 banyand/internal/storage.(*DiskMonitor).Stop(...) banyand/measure.(*standalone).GracefulStop(...) at measure/svc_standalone.go:347 ... run.(*Group).Run ... ``` `standalone.GracefulStop` runs `diskMonitor.Stop()` **before** `schemaRepo.Close()`. Because `DiskMonitor.Stop()` was stuck, `schemaRepo.Close()` never ran, so the TopN managers were never closed and the whole server (TopN flows, storage rotation tasks, gRPC serializers, `oklog/run` actors) stayed alive. The test harness's shutdown helper (`pkg/test/setup.CMD`) gives graceful stop 30s before abandoning it, so the `It` returned with the entire server still running and the `AfterEach` leak check tripped. ### The actual bug: `DiskMonitor.Stop()` busy-waits on a flag its loop can't clear ```go func (dm *DiskMonitor) Stop() { ... close(dm.stopCh) // Wait for any active cleanup to finish for dm.isActive.Load() { // <-- busy-wait time.Sleep(100 * time.Millisecond) } } func (dm *DiskMonitor) monitorLoop(serviceName string) { for { select { case <-dm.stopCh: return // <-- exits WITHOUT clearing isActive case <-dm.ticker.C: dm.safeCheckAndCleanup(serviceName) // <-- only place isActive is cleared } } } ``` `isActive` is only ever set back to `false` from inside a cleanup cycle running in `monitorLoop`. But once `Stop()` closes `stopCh`, `monitorLoop` takes the `stopCh` branch and returns **without running another cleanup cycle**, so `isActive` stays `true` forever and `Stop()` spins in its `for dm.isActive.Load()` loop indefinitely. This is exactly why the failure is **flaky**: the hang happens only when a forced cleanup is active (`isActive == true`) at the moment `Stop()` is called. The `Forced TTL Cleanup` test deliberately drives the disk above the high watermark to activate forced cleanup, so it hits the window often; other runs, where cleanup had already finished (`isActive == false`) before shutdown, stopped cleanly and passed. It is a genuine production shutdown deadlock, not a test artifact: any standalone or data node whose forced retention cleanup is active during `GracefulStop` can hang. ## Fix `banyand/internal/storage/disk_monitor.go`: 1. Add a `doneCh` that `monitorLoop` closes when it exits. `Stop()` now waits on `<-dm.doneCh` instead of polling `isActive`. This is deterministic: it waits for the loop (and any in-flight cleanup cycle) to actually finish, and cannot hang on a flag the loop never clears. 2. `monitorLoop` clears `isActive` (and the `forced_retention_active` gauge) in a `defer` on exit, so the monitor is correctly reported inactive after stop. 3. `Start()` closes `doneCh` immediately on the disabled path (`CheckInterval <= 0`), so `Stop()` does not block when no loop was ever started. 4. The cooldown `time.Sleep` between segment deletions is now a `select { case <-time.After(cooldown): case <-dm.stopCh: }`, so a shutdown during cooldown returns promptly instead of blocking for the full interval. No timeouts were widened, no sleeps added, no assertions weakened, and no test retries introduced — the flakiness is removed by fixing the shutdown ordering deadlock at its source. ## Verification - New regression test `TestDiskMonitor_StopDoesNotHangWhenActive` marks the monitor active and asserts `Stop()` returns within a bounded time (it fails/hangs against the old code, passes with the fix). - `go test -race ./banyand/internal/storage/ -run TestDiskMonitor` — all pass. - The previously failing spec passes reliably under the race detector: `go test -race ./test/integration/standalone/other/ -ginkgo.focus="Forced TTL Cleanup"` — ran 4× (16–17s each), green every time, with the goroutine-leak `AfterEach` now succeeding. - `golangci-lint` (project-pinned v1.64.8) clean on `banyand/internal/storage`. -- 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]
