From d0250dd74d693c1d5c33b3312072191ab5f16187 Mon Sep 17 00:00:00 2001
From: David Turner <david.turner@elastic.co>
Date: Tue, 19 Jul 2022 10:42:53 +0100
Subject: [PATCH] Weaken assertion in TransportService#doStop (#88601)

We may concurrently register a handler for a (closed) connection to a
remote node while stopping. Such a handler will be completed immediately
on the sending path if not picked up in `doStop`, but sometimes it's
picked up in `doStop` anyway. This commit weakens the assertion to
account for this case too.

Closes #88112 Closes #88459 Closes #88597
---
 .../transport/TransportService.java             | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/transport/TransportService.java b/server/src/main/java/org/elasticsearch/transport/TransportService.java
index 1f614eaf42ed6..7d1aa070df616 100644
--- a/server/src/main/java/org/elasticsearch/transport/TransportService.java
+++ b/server/src/main/java/org/elasticsearch/transport/TransportService.java
@@ -313,22 +313,25 @@ protected void doStop() {
             throw new UncheckedIOException(e);
         } finally {
             // The underlying transport has stopped which closed all the connections to remote nodes and hence completed all their handlers,
-            // but there may still be pending handlers for node-local requests since this connection is not closed. We complete them here:
+            // but there may still be pending handlers for node-local requests since this connection is not closed, and we may also
+            // (briefly) track handlers for requests which are sent concurrently with stopping even though the underlying connection is
+            // now closed. We complete all these outstanding handlers here:
             for (final Transport.ResponseContext<?> holderToNotify : responseHandlers.prune(h -> true)) {
                 try {
                     final TransportResponseHandler<?> handler = holderToNotify.handler();
                     final var targetNode = holderToNotify.connection().getNode();
 
-                    // Assertion only holds for TcpTransport only because other transports (used in tests) may not implement the proper
-                    // close-connection behaviour. TODO fix this.
-                    assert transport instanceof TcpTransport == false || targetNode.equals(localNode)
+                    assert transport instanceof TcpTransport == false
+                        /* other transports (used in tests) may not implement the proper close-connection behaviour. TODO fix this. */
+                        || targetNode.equals(localNode)
+                        /* local node connection cannot be closed so may still have pending handlers */
+                        || holderToNotify.connection().isClosed()
+                        /* connections to remote nodes must be closed by this point but could still have pending handlers */
                         : "expected only responses for local "
                             + localNode
                             + " but found handler for ["
                             + holderToNotify.action()
-                            + "] on ["
-                            + (holderToNotify.connection().isClosed() ? "closed" : "open")
-                            + "] connection to "
+                            + "] on open connection to "
                             + targetNode;
 
                     final var exception = new SendRequestTransportException(