SOLR-17777: Fix leaked metric registry after core unload (#3382)

This fixes a minor memory leak when deleting a core. I don't think it can have a significant impact on heap, but the fix is trivial and not risky.

Author: psalagnac

solr/CHANGES.txt

@@ -339,6 +339,8 @@ Bug Fixes
 
 * SOLR-17711: Remove total request timeout during recovery that was inadvertently added. (Luke Kot-Zaniewski)
 
+* SOLR-17777: Fix a very minor memory leak in metrics reporting code when a core is deleted. (Pierre Salagnac)
+
 Dependency Upgrades
 ---------------------
 * SOLR-17471: Upgrade Lucene to 9.12.1. (Pierre Salagnac, Christine Poerschke)

solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java

@@ -448,19 +448,35 @@ private static boolean isSharedRegistry(String registry) {
   }
 
   /**
-   * Get (or create if not present) a named registry
+   * Get (or create if not present) a named registry. This method always creates and persists a new
+   * registry in case it does not already exist.
    *
    * @param registry name of the registry
    * @return existing or newly created registry
    */
   public MetricRegistry registry(String registry) {
+    return registry(registry, true);
+  }
+
+  /**
+   * Get (or create if not present) a named registry.
+   *
+   * @param registry name of the registry.
+   * @param create When false and the registry does not exist, we return null instead of creating a
+   *     new one.
+   */
+  public MetricRegistry registry(String registry, boolean create) {
     registry = enforcePrefix(registry);
     if (isSharedRegistry(registry)) {
       return SharedMetricRegistries.getOrCreate(registry);
     } else {
       swapLock.lock();
       try {
-        return getOrCreateRegistry(registries, registry);
+        if (create) {
+          return getOrCreateRegistry(registries, registry);
+        } else {
+          return registries.get(registry);
+        }
       } finally {
         swapLock.unlock();
       }
@@ -830,7 +846,7 @@ public int unregisterGauges(String registryName, String tagSegment) {
     if (tagSegment == null) {
       return 0;
     }
-    MetricRegistry registry = registry(registryName);
+    MetricRegistry registry = registry(registryName, false);
     if (registry == null) return 0;
     AtomicInteger removed = new AtomicInteger();
     registry.removeMatching(

solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java

@@ -199,7 +199,8 @@ public void test() throws Exception {
 
     // check replication metrics on this jetty - see SOLR-14924
     for (JettySolrRunner jetty : cluster.getJettySolrRunners()) {
-      if (jetty.getCoreContainer() == null) {
+      if (emptyNode.equals(jetty.getNodeName())) {
+        // No cores on this node, ignore it
         continue;
       }
       SolrMetricManager metricManager = jetty.getCoreContainer().getMetricManager();

solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java

@@ -179,7 +179,8 @@ public void test() throws Exception {
 
     // check replication metrics on this jetty - see SOLR-14924
     for (JettySolrRunner jetty : cluster.getJettySolrRunners()) {
-      if (jetty.getCoreContainer() == null) {
+      if (emptyNode.equals(jetty.getNodeName())) {
+        // No cores on this node, ignore it
         continue;
       }
       SolrMetricManager metricManager = jetty.getCoreContainer().getMetricManager();

solr/core/src/test/org/apache/solr/metrics/SolrCoreMetricManagerTest.java

@@ -23,14 +23,22 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Random;
+import java.util.Set;
 import java.util.stream.Collectors;
 import org.apache.lucene.tests.util.TestUtil;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.params.CoreAdminParams;
+import org.apache.solr.common.params.MapSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.params.UpdateParams;
+import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.PluginInfo;
+import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.SolrInfoBean;
 import org.apache.solr.metrics.reporters.MockMetricReporter;
+import org.apache.solr.request.SolrQueryRequestBase;
 import org.apache.solr.schema.FieldType;
+import org.apache.solr.update.CommitUpdateCommand;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -177,4 +185,33 @@ public void testNonCloudRegistryName() {
     assertEquals("solr.core.collection1", registryName);
     assertNull(leaderRegistryName);
   }
+
+  /** Check the metric registry specific to a core is removed once the core is unloaded. */
+  @Test
+  public void testNoRegistryAfterUnload() throws Exception {
+
+    String coreRegistryName;
+
+    CoreContainer cc = h.getCoreContainer();
+    SolrMetricManager metricManager = cc.getMetricManager();
+
+    SolrCore core = cc.create("to-unload", Map.of("configSet", "minimal"));
+    coreRegistryName = core.getSolrMetricsContext().getRegistryName();
+
+    Set<String> names = metricManager.registryNames();
+    assertTrue("missing registry", names.contains(coreRegistryName));
+
+    // Commit and wait for searcher to ensure the searcher is created. This is required to make sure
+    // the core inner thread does not create it *after* we asked the container to unload the core.
+    SolrParams params = new MapSolrParams(Map.of(UpdateParams.WAIT_SEARCHER, "true"));
+    core.getUpdateHandler()
+        .commit(new CommitUpdateCommand(new SolrQueryRequestBase(h.getCore(), params) {}, false));
+
+    cc.unload("to-unload");
+
+    // Make sure the core metric registry does not exist anymore once we fully removed the core
+    // from the container
+    names = metricManager.registryNames();
+    assertFalse(names.contains(coreRegistryName));
+  }
 }