alertmanager icon indicating copy to clipboard operation
alertmanager copied to clipboard

Fix goroutine leak at silence bulk import

Open moonyoungCHAE opened this issue 3 months ago • 3 comments

In silence bulk import, I noticed that if a decoding error occurs, the number of goroutines remains equal to the number of workers which is worker count. This happened because the channel wasn’t closed when an error occurred and the function exited. I fixed this issue by ensuring the channel is closed even in error cases.

I tested like this.

func TestName(t *testing.T) {
	var err error
	alertmanagerURL, err = url.Parse("http://example.com")
	if err != nil {
		log.Fatal(err)
	}

	goroutineCount := runtime.NumGoroutine()
	println("before bulk import ", goroutineCount) // print 2
	cmd := silenceImportCmd{
		force:   false,
		workers: 10,
		file:    "./test.json",
	}
	cmd.bulkImport(context.TODO(), nil)

	for i := 0; i < 100; i++ {
		goroutineCount := runtime.NumGoroutine()
		println(goroutineCount) // as-is: 13 (worker count), to-be: 2
		time.Sleep(1 * time.Second)
	}
}
[
  {
    "ID": "123",
    "Comment": "First silence",
    "CreatedBy": "user1",
    "EndsAt": 1 // this makes decode error
  },
  {
    "ID": "456",
    "Comment": "Second silence",
    "CreatedBy": "user2"
  }
]

moonyoungCHAE avatar Sep 12 '25 06:09 moonyoungCHAE

I am 99.9% sure the test failing is unrelated. I think a real maintainer can rerun that before merging.

Spaceman1701 avatar Nov 05 '25 20:11 Spaceman1701

LGTM, let's re-run the tests :)

ultrotter avatar Nov 10 '25 16:11 ultrotter

@ultrotter @Spaceman1701 Thanks for the comment! I rerun the test and all checks have passed.

moonyoungCHAE avatar Nov 11 '25 01:11 moonyoungCHAE