Merge "Fix returning trace ID to client on failure after auto retry with trace"

java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@@ -166,6 +166,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
@@ -280,6 +281,7 @@ public class RestApiServlet extends HttpServlet {
 
   private final Globals globals;
   private final Provider<RestCollection<RestResource, RestResource>> members;
+  private Optional<String> traceId;
 
   public RestApiServlet(
       Globals globals, RestCollection<? extends RestResource, ? extends RestResource> members) {
@@ -551,7 +553,8 @@ public class RestApiServlet extends HttpServlet {
             throw new ResourceNotFoundException();
           }
 
-          response.traceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+          traceId = response.traceId();
+          traceId.ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
 
           if (response instanceof Response.Redirect) {
             CacheHeaders.setNotCacheable(res);
@@ -1485,11 +1488,12 @@ public class RestApiServlet extends HttpServlet {
     }
   }
 
-  private static long handleException(
-      Throwable err, HttpServletRequest req, HttpServletResponse res) throws IOException {
+  private long handleException(Throwable err, HttpServletRequest req, HttpServletResponse res)
+      throws IOException {
     logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
     if (!res.isCommitted()) {
       res.reset();
+      traceId.ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
       return replyError(req, res, SC_INTERNAL_SERVER_ERROR, "Internal server error", err);
     }
     return 0;

java/com/google/gerrit/server/restapi/change/Submit.java

@@ -19,6 +19,7 @@ import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Sets;
@@ -183,13 +184,13 @@ public class Submit
     }
     projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
 
-    return Response.ok(new Output(mergeChange(rsrc, submitter, input)));
+    return mergeChange(rsrc, submitter, input);
   }
 
   @UsedAt(UsedAt.Project.GOOGLE)
-  public Change mergeChange(RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input)
-      throws RestApiException, IOException, UpdateException, ConfigInvalidException,
-          PermissionBackendException {
+  public Response<Output> mergeChange(
+      RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input)
+      throws RestApiException, IOException {
     Change change = rsrc.getChange();
     if (!change.isNew()) {
       throw new ResourceConflictException("change is " + ChangeUtil.status(change));
@@ -204,10 +205,17 @@ public class Submit
     }
 
     try (MergeOp op = mergeOpProvider.get()) {
-      Change updatedChange = op.merge(change, submitter, true, input, false);
+      Change updatedChange;
+
+      try {
+        updatedChange = op.merge(change, submitter, true, input, false);
+      } catch (Exception e) {
+        Throwables.throwIfInstanceOf(e, RestApiException.class);
+        return Response.<Output>internalServerError(e).traceId(op.getTraceId().orElse(null));
+      }
 
       if (updatedChange.isMerged()) {
-        return change;
+        return Response.ok(new Output(change));
       }
 
       String msg =
@@ -462,8 +470,14 @@ public class Submit
         throw new ResourceConflictException("current revision is missing");
       }
 
-      Output out = submit.apply(new RevisionResource(rsrc, ps), input).value();
-      return Response.ok(json.noOptions().format(out.change));
+      Response<Output> response = submit.apply(new RevisionResource(rsrc, ps), input);
+      if (response instanceof Response.InternalServerError) {
+        Response.InternalServerError<?> ise = (Response.InternalServerError<?>) response;
+        return Response.<ChangeInfo>internalServerError(ise.cause())
+            .traceId(ise.traceId().orElse(null));
+      }
+
+      return Response.ok(json.noOptions().format(response.value().change));
     }
   }
 }

java/com/google/gerrit/server/submit/MergeOp.java

@@ -91,6 +91,7 @@ import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
@@ -247,6 +248,7 @@ public class MergeOp implements AutoCloseable {
   private Set<Project.NameKey> allProjects;
   private boolean dryrun;
   private TopicMetrics topicMetrics;
+  private String traceId;
 
   @Inject
   MergeOp(
@@ -518,6 +520,7 @@ public class MergeOp implements AutoCloseable {
                         .multipliedBy(cs.projects().size()))
                 .caller(getClass())
                 .retryWithTrace(t -> !(t instanceof RestApiException))
+                .onAutoTrace(traceId -> this.traceId = traceId)
                 .build());
 
         if (projects > 1) {
@@ -538,6 +541,10 @@ public class MergeOp implements AutoCloseable {
     }
   }
 
+  public Optional<String> getTraceId() {
+    return Optional.ofNullable(traceId);
+  }
+
   private void openRepoManager() {
     if (orm != null) {
       orm.close();

javatests/com/google/gerrit/acceptance/rest/TraceIT.java

@@ -585,17 +585,18 @@ public class TraceIT extends AbstractDaemonTest {
     assertThat(projectCreationListener.tags.get("project")).containsExactly("new24");
   }
 
+  @Test
   @GerritConfig(name = "retry.retryWithTraceOnFailure", value = "true")
   public void autoRetryWithTrace() throws Exception {
     String changeId = createChange().getChangeId();
     approve(changeId);
 
     TraceSubmitRule traceSubmitRule = new TraceSubmitRule();
-    traceSubmitRule.failOnce = true;
+    traceSubmitRule.failAlways = true;
     RegistrationHandle submitRuleRegistrationHandle = submitRules.add("gerrit", traceSubmitRule);
     try {
       RestResponse response = adminRestSession.post("/changes/" + changeId + "/submit");
-      assertThat(response.getStatusCode()).isEqualTo(SC_OK);
+      assertThat(response.getStatusCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR);
       assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).startsWith("retry-on-failure-");
       assertThat(traceSubmitRule.traceId).startsWith("retry-on-failure-");
       assertThat(traceSubmitRule.isLoggingForced).isTrue();
@@ -617,7 +618,7 @@ public class TraceIT extends AbstractDaemonTest {
       assertThat(response.getStatusCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR);
       assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
       assertThat(traceSubmitRule.traceId).isNull();
-      assertThat(traceSubmitRule.isLoggingForced).isNull();
+      assertThat(traceSubmitRule.isLoggingForced).isFalse();
     } finally {
       submitRuleRegistrationHandle.remove();
     }
@@ -677,18 +678,19 @@ public class TraceIT extends AbstractDaemonTest {
     String traceId;
     Boolean isLoggingForced;
     boolean failOnce;
+    boolean failAlways;
 
     @Override
     public Optional<SubmitRecord> evaluate(ChangeData changeData) {
-      if (failOnce) {
-        failOnce = false;
-        throw new IllegalStateException("forced failure from test");
-      }
-
       this.traceId =
           Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null);
       this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false);
 
+      if (failOnce || failAlways) {
+        failOnce = false;
+        throw new IllegalStateException("forced failure from test");
+      }
+
       SubmitRecord submitRecord = new SubmitRecord();
       submitRecord.status = SubmitRecord.Status.OK;
       return Optional.of(submitRecord);