SOLR-17875: Ensure SolrMetricsContext is closed by dsmiley · Pull Request #4226 · apache/solr

dsmiley · 2026-03-18T14:16:35Z

SolrMetricsContext is now AutoCloseable. Renamed unregister() to close().

https://issues.apache.org/jira/browse/SOLR-17875

Ensure all SolrMetricsContext instances are properly closed to prevent resource leaks
Add ObjectReleaseTracker tracking to SolrMetricsContext constructors so leaked instances cause test failures
Add registerCloseable(AutoCloseable) to SolrMetricsContext for lifecycle-managing related resources (e.g. AttributedInstrumentFactory's node-level context)
Fix several production code bugs where SolrMetricsContext was never closed:
- RequestHandlerBase.initializeMetrics(): close old context on re-registration instead of asserting null
- AuditLoggerPlugin.close(): SolrInfoBean.super.close() was guarded by async-only condition
- Metrics (blockcache): close() override didn't close its metrics context
- AttributedInstrumentFactory: node-level SolrMetricsContext was never closed
- CaffeineCache/ThinCache: NPE on close() when not fully initialized
Fix ~10 test suites that leaked SolrMetricsContext instances

SolrMetricsContext is now AutoCloseable. Renamed unregister() to close().

dsmiley · 2026-03-18T14:17:48Z

solr/core/src/java/org/apache/solr/core/CoreContainer.java


      customThreadPool.execute(replayUpdatesExecutor::shutdownAndAwaitTermination);

-      // Shutdown GPU metrics service if it was initialized


Shutdown/close should do its work in reverse order of construction. Since metrics is initialized super early, I moved the compensating shutdown logic to the end.

dsmiley · 2026-03-18T14:18:16Z

solr/core/src/java/org/apache/solr/core/CoreContainer.java

+
+    shutdownGpuMetricsService(); // Shutdown GPU metrics service if it was initialized
+
+    IOUtils.closeQuietly(solrMetricsContext);


this line is new

dsmiley · 2026-03-18T14:18:22Z

solr/core/src/java/org/apache/solr/core/CoreContainer.java

    }
    org.apache.lucene.util.IOUtils.closeWhileHandlingException(loader); // best effort
+
+    containerHandlers.close();


this line is new

dsmiley · 2026-03-18T14:19:26Z

solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java

  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

-  private HandlerMetrics metricsShard = HandlerMetrics.NO_OP;
+  private HandlerMetrics metricsShard; // nocommit NO_OP


i'll remove the nocommit ; I wanted to see if we could eliminate NO_OP entirely, and we can.

I'm more convinced than ever that the NO_OP was a bad move. It was making problems that should have been root-caused. Handlers cannot be used until they have been initialized (including metrics). A follow-up commit here will show that InfoHandler was creating sub-handlers and not fully initializing them, thus any metrics such sub-handlers wanted to create were not getting closed. PublicKeyHandler seems affected as well.

dsmiley · 2026-03-18T14:20:10Z

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java


  protected boolean publishCpuTime = Boolean.getBoolean(ThreadCpuTimer.ENABLE_CPU_TIME);

-  @SuppressForbidden(reason = "Need currentTimeMillis, used only for stats output")


dsmiley · 2026-03-18T14:21:03Z

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java


  @Override
  public void initializeMetrics(SolrMetricsContext parentContext, Attributes attributes) {
+    IOUtils.closeQuietly(this.solrMetricsContext);


Unsure of this... I suppose if it's already set. Lemme see why that could be...
Ah; it's not needed. Will replace with a comment.

dsmiley · 2026-03-18T14:59:38Z

solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java

      this.nodeMetricsContext =
          new SolrMetricsContext(
              primaryMetricsContext.getMetricManager(), SolrMetricManager.NODE_REGISTRY);
+      primaryMetricsContext.registerCloseable(this.nodeMetricsContext);


Admittedly, this is the only non-test caller of the registerCloseable method I added

dsmiley · 2026-03-18T15:12:34Z

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

+    assert ObjectReleaseTracker.track(this);
+  }
+
+  public SolrMetricsContext(SolrMetricManager metricManager, String registryName, Object child) {


more info helps debug if there's a failure

dsmiley · 2026-03-18T15:19:34Z

solr/core/src/java/org/apache/solr/update/UpdateHandler.java


+  @Override
+  public void close() throws IOException {
+    IOUtils.closeQuietly(ulog);


this was a gap; not sure of the consequences

HoustonPutman

Small comments, overall looks good to me, and it should be easy to tell if the tests lose all the cruft in the logging.

HoustonPutman · 2026-03-18T18:02:55Z

solr/core/src/test/org/apache/solr/blockcache/BufferStoreTest.java

  @After
  public void clearBufferStores() {
    BufferStore.clearBufferStores();
+    org.apache.solr.common.util.IOUtils.closeQuietly(metrics);


Feel like this can be imported...

HoustonPutman · 2026-03-18T18:04:15Z

solr/core/src/test/org/apache/solr/search/TestThinCache.java

    String lfuCacheName = "lfu_cache";
    lfuCache.setBacking(cacheScope, backing);
-    SolrMetricsContext solrMetricsContext = new SolrMetricsContext(metricManager, registry);
+    var solrMetricsContext = new SolrMetricsContext(metricManager, registry);


Best to put this in a try-with-resources so that we don't get leaks on a failure.

I had thought about that. Rather than be super anal about this detail in tests, which is annoying, I'd rather we configure the ORT mechanism to just log a one-liner warning when the test fails, since if a test fails then there's generally a decent chance that something is not closed. I'll do a separate PR for that.

Hmm; I suppose for some time, ObjectReleaseTracker won't complain if the test fails (for other reasons, not due to itself). The code only checks when afterIsSuccessful So nothing more to do here.

is called before handling requests, preventing memory leaks from untracked SolrMetricsContext instances. Fix PluginBag to track all plugins (V1, V2, and defaults) in allPlugins list, ensuring proper cleanup during replacement and shutdown. Make the list thread-safe when needed. Update InfoHandler and SchemaHandler to initialize metrics on dynamically-created sub-handlers, registering them for cleanup. Fix tests that create handlers directly to initialize metrics and close handlers in tearDown (InfoHandlerTest, MaxSizeAutoCommitTest). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dsmiley

AFAIK the only issue left is {{org.apache.solr.cloud.MissingSegmentRecoveryTest}} which has proved a bit of a nightmare for Claude Opus ... so much so that the best action may be to figure out how to conditionally disable the SolrMetricsContext tracking for just this test.

solr/core/src/java/org/apache/solr/core/SolrCore.java

dsmiley · 2026-03-19T13:29:43Z

solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java

        solrMetricsContext.instrumentedExecutorService(
            coreAdminAsyncTracker.expensiveExecutor,
-            "solr_node_executor",
+            "solr_node_expensive_executor",


Oh I forgot to mention this, this is not the only instance of this. There are at least a few others. And I wanted to check with you or @mlbiscoc to see if this was the right kind of fix.

dsmiley · 2026-03-19T13:30:21Z

solr/core/src/java/org/apache/solr/handler/admin/InfoHandler.java

InfoHandler has sub-handlers, and must tend to their lifecycle, including metrics

dsmiley · 2026-03-19T13:32:07Z

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

BTW I really appreciate the improvements here @HoustonPutman :-)

dsmiley · 2026-03-19T13:33:47Z

solr/core/src/java/org/apache/solr/handler/SchemaHandler.java


+  @Override
+  public void initializeMetrics(SolrMetricsContext parentContext, Attributes attributes) {
+    // Store parent context so we can use it in inform() to initialize sub-handlers as siblings


I wish inform() calls happened before initializeMetrics... but that's another issue and isn't easy

Yeah, I agree.

On a somewhat related note, I didn't want to go and change everything here, but I really really wished that initializeMetrics worked the same across all classes. It seems like half of the classes that implement it treat parentContext like its own context (and generally in those cases initializeMetrics is called by the constructor which creates the childContext itself), and then half actually treat it like a parentContext and initialize a childContext in the method. This can get quite confusing trying to figure out where and how these contexts are created and managed.

Totally agree! Worth a JIRA IMO.

SOLR-17875: Ensure SolrMetricsContext is closed

031e9b7

SolrMetricsContext is now AutoCloseable. Renamed unregister() to close().

dsmiley requested a review from mlbiscoc March 18, 2026 14:16

github-actions bot added client:solrj module:jwt-auth tests cat:search cat:cloud cat:index cat:security cat:metrics labels Mar 18, 2026

dsmiley commented Mar 18, 2026

View reviewed changes

HoustonPutman approved these changes Mar 18, 2026

View reviewed changes

HoustonPutman and others added 2 commits March 18, 2026 13:37

Make sure we aren't closing metrics more than once

4e53226

dsmiley commented Mar 19, 2026

View reviewed changes

dsmiley added 5 commits March 20, 2026 21:12

Remove SolrFieldCache from core metrics

4a32ff2

changelog

e852bab

No need to collect Closeables here.

ad47ac0

Hack/work-around for MissingSegmentRecoveryTest

b298f82

SolrFieldCacheBean: fix recursive close

60dcb39


		customThreadPool.execute(replayUpdatesExecutor::shutdownAndAwaitTermination);

		// Shutdown GPU metrics service if it was initialized


		shutdownGpuMetricsService(); // Shutdown GPU metrics service if it was initialized

		IOUtils.closeQuietly(solrMetricsContext);


		protected boolean publishCpuTime = Boolean.getBoolean(ThreadCpuTimer.ENABLE_CPU_TIME);

		@SuppressForbidden(reason = "Need currentTimeMillis, used only for stats output")

Conversation

dsmiley commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoustonPutman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoustonPutman Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HoustonPutman Mar 19, 2026 •

edited

Loading