[-] resolve monitoredSources race condition, fixes #1213

[Mazen050]

Fix a data race in Reaper caused by concurrent access to the shared monitoredSources slice.

LoadSources() updates this slice while background goroutines (Reap() loop and WriteMonitoredSources()) read it concurrently, which triggers race detector warnings.

This PR protects the slice with an RWMutex and clones it before iteration to ensure safe concurrent access.

Fixes #1213

[pashagolub]

It would be perfect if you can add some tests

[pashagolub]

I'm not sure this file is somehow related to the fix

[Mazen050]

Sorry that's my bad, I forgot to mention that this specific test failed in CI after the fix, Also it didn't fail on my machine so its related to github's CI.

[Mazen050]

I attempted to fix it so that when you review we can see if my fix is good or we should take another approach.

[Mazen050]

It would be perfect if you can add some tests

Sure thing, I was actually planning on adding tests for this specific file and had some tests already written. Will add these here asap.

[Mazen050]

Hello @pashagolub

I have added tests increasing coverage by about 9%. Only two functions remaining with 0% coverage reapMetricMeasurements and Reap which both I think are hard. please let me know if we should make tests for them so that I can try again with them and either send another PR or add to this. Also if we were to make a test for Reap how would we go about it?

Also please let me know if there are any edge cases we can add tests for that are not necessarily related to coverage.

[0xgouda]

@pashagolub, I wonder if, instead of adding additional locking overhead, we should deprecate configured_dbs and remove the writeMonitoredSources() function entirely. Almost all dashboards now rely on admin.all_distinct_dbname_metrics instead?

[pashagolub]

@pashagolub, I wonder if, instead of adding additional locking overhead, we should deprecate configured_dbs and remove the writeMonitoredSources() function entirely. Almost all dashboards now rely on admin.all_distinct_dbname_metrics instead?

We use it in Global Health dashboards. If we can get rid of it, I will support this decision