fix: polling policy stops after 'too many' successes (#1427)

Author: coryan

google/cloud/spanner/internal/polling_loop.h

@@ -130,11 +130,23 @@ typename ValueExtractor::ReturnType PollingLoopImpl(
     google::longrunning::GetOperationRequest poll_request;
     poll_request.set_name(operation.name());
     auto update = functor(poll_context, poll_request);
-    if (!update) {
-      if (!polling_policy->OnFailure(update.status())) {
-        return std::move(update).status();
+    if (update && update->done()) {
+      // Before updating the polling policy make sure we do not discard a
+      // successful result that completes the request.
+      using std::swap;
+      swap(*update, operation);
+      break;
+    }
+    // Update the polling policy even on successful requests, so we can stop
+    // after too many polling attempts.
+    if (!polling_policy->OnFailure(update.status())) {
+      if (update) {
+        return Status(StatusCode::kDeadlineExceeded,
+                      "exhausted polling policy with no previous error");
       }
-    } else {
+      return std::move(update).status();
+    }
+    if (update) {
       using std::swap;
       swap(*update, operation);
     }

google/cloud/spanner/internal/polling_loop_test.cc

@@ -280,6 +280,23 @@ TEST(PollingLoopTest, FailureTooManyTransients) {
   EXPECT_EQ(StatusCode::kUnavailable, actual.status().code());
 }
 
+TEST(PollingLoopTest, FailureTooManySuccesses) {
+  google::longrunning::Operation operation;
+  operation.set_name("test-operation");
+  operation.set_done(false);
+
+  StatusOr<google::protobuf::Value> actual =
+      PollingLoop<PollingLoopResponseExtractor<google::protobuf::Value>>(
+          TestPollingPolicy(),
+          [&operation](grpc::ClientContext&,
+                       google::longrunning::GetOperationRequest const&) {
+            return StatusOr<google::longrunning::Operation>(operation);
+          },
+          operation, "location");
+  EXPECT_EQ(StatusCode::kDeadlineExceeded, actual.status().code());
+  EXPECT_THAT(actual.status().message(), HasSubstr("exhausted polling policy"));
+}
+
 TEST(PollingLoopTest, FailureMissingResponseAndError) {
   google::longrunning::Operation operation;
   operation.set_name("test-operation");