Clear headers on error during handler execution (#5122)

include/ccf/rpc_context.h

@@ -137,6 +137,7 @@ namespace ccf
     {
       set_response_header(kv.first, kv.second);
     }
+    virtual void clear_response_headers() = 0;
 
     virtual void set_response_trailer(
       const std::string_view& name, const std::string_view& value) = 0;

src/http/http_rpc_context.h

@@ -250,6 +250,11 @@ namespace http
       response_headers[std::string(name)] = value;
     }
 
+    virtual void clear_response_headers() override
+    {
+      response_headers.clear();
+    }
+
     virtual void set_response_trailer(
       const std::string_view& name, const std::string_view& value) override
     {

src/node/rpc/frontend.h

@@ -517,6 +517,7 @@ namespace ccf
               ctx->get_request_verb().c_str(),
               ctx->get_request_path());
 
+            ctx->clear_response_headers();
             ctx->set_error(
               HTTP_STATUS_INTERNAL_SERVER_ERROR,
               ccf::errors::InternalError,
@@ -566,6 +567,7 @@ namespace ccf
                 catch (const std::exception& e)
                 {
                   // run default handler to set transaction id in header
+                  ctx->clear_response_headers();
                   ccf::endpoints::default_locally_committed_func(
                     args, tx_id.value());
                   ctx->set_error(
@@ -578,6 +580,7 @@ namespace ccf
                 catch (...)
                 {
                   // run default handler to set transaction id in header
+                  ctx->clear_response_headers();
                   ccf::endpoints::default_locally_committed_func(
                     args, tx_id.value());
                   ctx->set_error(
@@ -605,6 +608,7 @@ namespace ccf
 
             case kv::CommitResult::FAIL_NO_REPLICATE:
             {
+              ctx->clear_response_headers();
               ctx->set_error(
                 HTTP_STATUS_SERVICE_UNAVAILABLE,
                 ccf::errors::TransactionReplicationFailed,
@@ -624,12 +628,14 @@ namespace ccf
         }
         catch (RpcException& e)
         {
+          ctx->clear_response_headers();
           ctx->set_error(std::move(e.error));
           update_metrics(ctx);
           return;
         }
         catch (const JsonParseError& e)
         {
+          ctx->clear_response_headers();
           ctx->set_error(
             HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput, e.describe());
           update_metrics(ctx);
@@ -637,6 +643,7 @@ namespace ccf
         }
         catch (const nlohmann::json::exception& e)
         {
+          ctx->clear_response_headers();
           ctx->set_error(
             HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput, e.what());
           update_metrics(ctx);
@@ -653,6 +660,7 @@ namespace ccf
         }
         catch (const std::exception& e)
         {
+          ctx->clear_response_headers();
           ctx->set_error(
             HTTP_STATUS_INTERNAL_SERVER_ERROR,
             ccf::errors::InternalError,
@@ -662,6 +670,7 @@ namespace ccf
         }
       } // end of while loop
 
+      ctx->clear_response_headers();
       ctx->set_error(
         HTTP_STATUS_SERVICE_UNAVAILABLE,
         ccf::errors::TransactionCommitAttemptsExceedLimit,
@@ -669,6 +678,7 @@ namespace ccf
           "Transaction continued to conflict after {} attempts. Retry "
           "later.",
           max_attempts));
+      update_metrics(ctx);
       static constexpr size_t retry_after_seconds = 3;
       ctx->set_response_header(http::headers::RETRY_AFTER, retry_after_seconds);
 

src/node/rpc/test/frontend_test.cpp

@@ -1491,6 +1491,7 @@ class TestConflictFrontend : public BaseTestFrontend
 {
 public:
   using Values = kv::Map<size_t, size_t>;
+  size_t execution_count = 0;
 
   TestConflictFrontend(kv::Store& tables) : BaseTestFrontend(tables)
   {
@@ -1502,8 +1503,6 @@ public:
                     .value()); // This header only exists in the context of
                                // this test
 
-      static size_t execution_count = 0;
-
       auto conflict_map = ctx.tx.template rw<Values>("test_values_conflict");
       conflict_map->get(0); // Record a read dependency
 
@@ -1547,6 +1546,7 @@ TEST_CASE("Retry on conflict")
 
   INFO("Does not reach execution limit");
   {
+    frontend.execution_count = 0;
     size_t retry_count = ccf_max_attempts - 1;
     req.set_header("test-retry-count", fmt::format("{}", retry_count));
     auto serialized_call = req.build_request();
@@ -1564,6 +1564,7 @@ TEST_CASE("Retry on conflict")
 
   INFO("Reaches execution limit");
   {
+    frontend.execution_count = 0;
     size_t retry_count = ccf_max_attempts + 1;
     req.set_header("test-retry-count", fmt::format("{}", retry_count));
     auto serialized_call = req.build_request();
@@ -1573,10 +1574,10 @@ TEST_CASE("Retry on conflict")
     auto response = parse_response(rpc_ctx->serialise_response());
     CHECK(response.status == HTTP_STATUS_SERVICE_UNAVAILABLE);
 
-    CHECK(response.headers["test-has-conflicted"] == "true");
+    // Response headers are cleared on error
     CHECK(
-      response.headers["test-execution-count"] ==
-      fmt::format("{}", ccf_max_attempts));
+      response.headers.find("test-has-conflicted") == response.headers.end());
+    CHECK(frontend.execution_count == ccf_max_attempts);
   }
 }