SOLR-12477: Return server error(500) for AlreadyClosedException instead of client Errors(400) . This closes PR #402

solr/CHANGES.txt

@@ -181,6 +181,9 @@ Bug Fixes
 
 * SOLR-12597: Migrate API should fail requests that do not specify split.key parameter (shalin)
 
+* SOLR-12477: An update would return a client error(400) if it hit a AlreadyClosedException.
+  We now return the error as a server error(500) instead (Jeffery via Varun Thacker)
+
 Optimizations
 ----------------------
 

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

@@ -1806,8 +1806,12 @@ public class CoreContainer {
     return false;
   }
 
-  public void checkTragicException(SolrCore solrCore) {
-    Throwable tragicException = null;
+  /**
+   * @param solrCore te core against which we check if there has been a tragic exception
+   * @return whether this solr core has tragic exception
+   */
+  public boolean checkTragicException(SolrCore solrCore) {
+    Throwable tragicException;
     try {
       tragicException = solrCore.getSolrCoreState().getTragicException();
     } catch (IOException e) {
@@ -1820,6 +1824,8 @@ public class CoreContainer {
         getZkController().giveupLeadership(solrCore.getCoreDescriptor(), tragicException);
       }
     }
+    
+    return tragicException != null;
   }
 
 }

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

@@ -209,7 +209,16 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
       }
     } catch (Exception e) {
       if (req.getCore() != null) {
-        req.getCore().getCoreContainer().checkTragicException(req.getCore());
+        boolean isTragic = req.getCore().getCoreContainer().checkTragicException(req.getCore());
+        if (isTragic) {
+          if (e instanceof SolrException) {
+            // Tragic exceptions should always throw a server error
+            assert ((SolrException) e).code() == 500;
+          } else {
+            // wrap it in a solr exception
+            e = new SolrException(SolrException.ErrorCode.SERVER_ERROR, e.getMessage(), e);
+          }
+        }
       }
       boolean incrementErrors = true;
       boolean isServerError = true;

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

@@ -41,6 +41,7 @@ import org.apache.lucene.search.BooleanQuery;
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.search.TermQuery;
+import org.apache.lucene.store.AlreadyClosedException;
 import org.apache.lucene.util.BytesRefHash;
 import org.apache.solr.cloud.ZkController;
 import org.apache.solr.common.SolrException;
@@ -233,6 +234,9 @@ public class DirectUpdateHandler2 extends UpdateHandler implements SolrCoreState
       return addDoc0(cmd);
     } catch (SolrException e) {
       throw e;
+    } catch (AlreadyClosedException e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+          String.format(Locale.ROOT, "Server error writing document id %s to the index", cmd.getPrintableId()), e);
     } catch (IllegalArgumentException iae) {
       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
           String.format(Locale.ROOT, "Exception writing document id %s to the index; possible analysis error: "

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

@@ -17,18 +17,23 @@
 
 package org.apache.solr.cloud;
 
+import static org.hamcrest.CoreMatchers.anyOf;
+import static org.hamcrest.CoreMatchers.is;
+
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-
+import org.apache.lucene.store.AlreadyClosedException;
 import org.apache.lucene.store.MockDirectoryWrapper;
 import org.apache.solr.client.solrj.embedded.JettySolrRunner;
 import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.ClusterStateUtil;
 import org.apache.solr.common.cloud.DocCollection;
 import org.apache.solr.common.cloud.Replica;
@@ -133,8 +138,17 @@ public class LeaderTragicEventTest extends SolrCloudTestCase {
         }
       }
     } catch (Exception e) {
-      log.info("Corrupt leader ex: ",e);
-      // Expected
+      log.info("Corrupt leader ex: ", e);
+      
+      // solrClient.add/commit would throw RemoteSolrException with error code 500 or 
+      // 404(when the leader replica is already deleted by giveupLeadership)
+      if (e instanceof RemoteSolrException) {
+        SolrException se = (SolrException) e;
+        assertThat(se.code(), anyOf(is(500), is(404)));
+      } else if (!(e instanceof AlreadyClosedException)) {
+        throw new RuntimeException("Unexpected exception", e);
+      }
+      //else expected
     }
     return oldLeader;
   }