[Bug]: Goroutine leak in plugin/storage/es/

[yurishkuro]

What happened?

Unrelated PR failed with goleak:

Runtime error
goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 106 in state select, with github.com/olivere/elastic.(*Client).healthchecker on top of the stack:
github.com/olivere/elastic.(*Client).healthchecker(0xc0002561a0)
	/home/runner/go/pkg/mod/github.com/olivere/elastic@v6.2.37+incompatible/client.go:1060 +0x229
created by github.com/olivere/elastic.DialContext in goroutine 102
	/home/runner/go/pkg/mod/github.com/olivere/elastic@v6.2.37+incompatible/client.go:404 +0xcfa
 Goroutine 107 in state select, with github.com/olivere/elastic.(*bulkWorker).work on top of the stack:
github.com/olivere/elastic.(*bulkWorker).work(0xc000476180, {0x1de9fb8, 0x2906f40})
	/home/runner/go/pkg/mod/github.com/olivere/elastic@v6.2.37+incompatible/bulk_processor.go:480 +0x1c5
created by github.com/olivere/elastic.(*BulkProcessor).Start in goroutine 102
	/home/runner/go/pkg/mod/github.com/olivere/elastic@v6.2.37+incompatible/bulk_processor.go:336 +0x5dc
]

Steps to reproduce

Unknown, this appears to be a spurious condition when ES clients are not being stopped properly (they kick off a bg healthcheck go routine). It's possible that there is a race condition between starting and stopping the client, since we already spent time fixing these go leaks.

Expected behavior

No go leaks in the package.

[akagami-harsh]

I'll look into it

[akagami-harsh]

i think, this error could be because of Elasticsearch client is not properly closed at some parts of code in plugin/storage/es/factory_test.go

The Close() method of the Factory struct wasn't invoked at the end of some tests. While calling f.Close() might resolve the issue, I'm unable to confirm this without a reliable method of reproducing the error.

[yurishkuro]

@akagami-harsh where do you see specific places where Close is not called?

[yurishkuro]

One possible approach is to switch the tests to use this constructor from olivere lib:

func NewSimpleClient(options ...ClientOptionFunc) (*Client, error) {

It does not start any bg routines so would be more suitable for tests, but it will also introduce an extra complexity (e.g. in pkg/es) and divergence between prod and test behavior. Ideally we should track down if we're leaking a client when passwords are being rotated (the failures started after that code was merged).

[akagami-harsh]

@akagami-harsh where do you see specific places where Close is not called?

in TestArchiveEnabled

but the test passes without calling f.Close()

[yurishkuro]

Yes, there should be f.Close there, but since it's using mock client it shouldn't affect anything (still worth adding close)