Fix SearchService.createContext exception handling (#46258)

An exception from the DefaultSearchContext constructor could leak a
searcher, causing future issues like shard lock obtained exceptions. The
underlying cause of the exception in the constructor has been fixed, but
as a safety precaution we also fix the exception handling in
createContext.

Closes #45378

Author: henningandersen

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -626,11 +626,12 @@ private DefaultSearchContext createSearchContext(ShardSearchRequest request, Tim
                 indexShard.shardId(), request.getClusterAlias(), OriginalIndices.NONE);
         Engine.Searcher searcher = indexShard.acquireSearcher(source);
 
-        final DefaultSearchContext searchContext = new DefaultSearchContext(idGenerator.incrementAndGet(), request, shardTarget,
-            searcher, clusterService, indexService, indexShard, bigArrays, threadPool::relativeTimeInMillis, timeout,
-            fetchPhase, clusterService.state().nodes().getMinNodeVersion());
         boolean success = false;
+        DefaultSearchContext searchContext = null;
         try {
+            searchContext = new DefaultSearchContext(idGenerator.incrementAndGet(), request, shardTarget,
+                searcher, clusterService, indexService, indexShard, bigArrays, threadPool::relativeTimeInMillis, timeout,
+                fetchPhase, clusterService.state().nodes().getMinNodeVersion());
             // we clone the query shard context here just for rewriting otherwise we
             // might end up with incorrect state since we are using now() or script services
             // during rewrite and normalized / evaluate templates etc.
@@ -641,6 +642,11 @@ private DefaultSearchContext createSearchContext(ShardSearchRequest request, Tim
         } finally {
             if (success == false) {
                 IOUtils.closeWhileHandlingException(searchContext);
+                if (searchContext == null) {
+                    // we handle the case where the DefaultSearchContext constructor throws an exception since we would otherwise
+                    // leak a searcher and this can have severe implications (unable to obtain shard lock exceptions).
+                    IOUtils.closeWhileHandlingException(searcher);
+                }
             }
         }
         return searchContext;

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java

@@ -30,6 +30,7 @@
 import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.search.SearchTask;
+import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.action.support.PlainActionFuture;
 import org.elasticsearch.action.support.WriteRequest;
@@ -680,4 +681,28 @@ public void testCreateSearchContext() throws IOException {
             assertSame(searchShardTarget, searchContext.fetchResult().getSearchShardTarget());
         }
     }
+
+    /**
+     * While we have no NPE in DefaultContext constructor anymore, we still want to guard against it (or other failures) in the future to
+     * avoid leaking searchers.
+     */
+    public void testCreateSearchContextFailure() throws IOException {
+        final String index = randomAlphaOfLengthBetween(5, 10).toLowerCase(Locale.ROOT);
+        final IndexService indexService = createIndex(index);
+        final SearchService service = getInstanceFromNode(SearchService.class);
+        final ShardId shardId = new ShardId(indexService.index(), 0);
+
+        NullPointerException e = expectThrows(NullPointerException.class,
+            () -> service.createContext(
+                new ShardSearchLocalRequest(shardId, null, 0, null) {
+                    @Override
+                    public SearchType searchType() {
+                        // induce an artificial NPE
+                        throw new NullPointerException("expected");
+                    }
+                }
+            ));
+        assertEquals("expected", e.getMessage());
+        assertEquals("should have 2 store refs (IndexService + InternalEngine)", 2, indexService.getShard(0).store().refCount());
+    }
 }