[Sync] Fix sync scheduler/session logic determining successful commands.

We now have Succeeded(), which is true iff the command ran successfully, and
SuccessfullyReachedServer(), which is true iff we actually contacted the server
without errors.

BUG=131414
TEST=unit_tests


Review URL: https://chromiumcodereview.appspot.com/10537032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@140885 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 92 insertions, 29 deletions

diff --git a/sync/engine/sync_scheduler.cc b/sync/engine/sync_scheduler.cc
index 1e9da41..fa8dc52 100644
--- a/sync/engine/sync_scheduler.cc
+++ b/sync/engine/sync_scheduler.cc
@@ -881,13 +881,9 @@ void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
   AdjustPolling(&old_job);
 
   if (old_job.session->Succeeded()) {
-    // Success implies backoff relief.  Note that if this was a
-    // "one-off" job (i.e. purpose ==
-    // SyncSessionJob::{CLEAR_USER_DATA,CLEANUP_DISABLED_TYPES}), if
-    // there was work to do before it ran this wont have changed, as
-    // jobs like this don't run a full sync cycle.  So we don't need
-    // special code here.
-    wait_interval_.reset();
+    // Only reset backoff if we actually reached the server.
+    if (old_job.session->SuccessfullyReachedServer())
+      wait_interval_.reset();
     SDVLOG(2) << "Job succeeded so not scheduling more jobs";
     return;
   }
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 8028b43..c5de28e 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -733,6 +733,22 @@ TEST_F(SyncSchedulerTest, HasMoreToSync) {
   // cause our expectation to break.
 }
 
+// Test that continuations can go into backoff.
+TEST_F(SyncSchedulerTest, HasMoreToSyncThenFails) {
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+    .WillOnce(Invoke(sessions::test_util::SimulateHasMoreToSync))
+    .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+                    QuitLoopNowAction()));
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+  RunLoop();
+
+  scheduler()->ScheduleNudge(
+      zero(), NUDGE_SOURCE_LOCAL, ModelTypeSet(), FROM_HERE);
+
+  // We should detect the failure on the second sync share, and go into backoff.
+  EXPECT_TRUE(RunAndGetBackoff());
+}
+
 // Test that no syncing occurs when throttled.
 TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
   const ModelTypeSet types(syncable::BOOKMARKS);
@@ -862,7 +878,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
 
   EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1)
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
-          RecordSyncShareMultiple(&r, 1U)));
+                            RecordSyncShareMultiple(&r, 1U)));
   EXPECT_CALL(*delay(), GetDelay(_)).
       WillRepeatedly(Return(TimeDelta::FromDays(1)));
 
@@ -881,7 +897,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
 
   EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1)
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
-          RecordSyncShare(&r)));
+                      RecordSyncShare(&r)));
 
   // We schedule a nudge with enough delay (10X poll interval) that at least
   // one or two polls would have taken place.  The nudge should succeed.
@@ -894,12 +910,16 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
   EXPECT_EQ(GetUpdatesCallerInfo::LOCAL,
             r.snapshots[1].source().updates_source);
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(0);
+  // Cleanup is not affected by backoff, but it should not relieve it either.
+  EXPECT_CALL(*syncer(),
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   EXPECT_CALL(*delay(), GetDelay(_)).Times(0);
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
   RunLoop();
 
+  scheduler()->ScheduleCleanupDisabledTypes();
   scheduler()->ScheduleConfig(
       types, GetUpdatesCallerInfo::RECONFIGURATION);
   PumpLoop();
@@ -1053,7 +1073,7 @@ TEST_F(SyncSchedulerTest, GetRecommendedDelay) {
 TEST_F(SyncSchedulerTest, SyncerSteps) {
   // Nudges.
   EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END))
-      .Times(1);
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
   RunLoop();
 
@@ -1068,7 +1088,7 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
 
   // ClearUserData.
   EXPECT_CALL(*syncer(), SyncShare(_, CLEAR_PRIVATE_DATA, CLEAR_PRIVATE_DATA))
-      .Times(1);
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
   RunLoop();
 
@@ -1080,7 +1100,8 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   Mock::VerifyAndClearExpectations(syncer());
 
   // Configuration.
-  EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES));
+  EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
   RunLoop();
 
@@ -1094,7 +1115,8 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
 
   // Cleanup disabled types.
   EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES));
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
   RunLoop();
 
@@ -1109,7 +1131,8 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   // Poll.
   EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END))
       .Times(AtLeast(1))
-      .WillRepeatedly(QuitLoopNowAction());
+      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+                            QuitLoopNowAction()));
   const TimeDelta poll(TimeDelta::FromMilliseconds(10));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
 
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index ac6b5eb7..f8639c0 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -241,8 +241,10 @@ void Syncer::SyncShare(sessions::SyncSession* session,
              << "current step: " << SyncerStepToString(current_step) << ", "
              << "next step: " << SyncerStepToString(next_step) << ", "
              << "snapshot: " << session->TakeSnapshot().ToString();
-    if (last_step == current_step)
+    if (last_step == current_step) {
+      session->SetFinished();
       break;
+    }
     current_step = next_step;
   }
 }
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 85ac8cc..e219412 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -59,7 +59,8 @@ SyncSession::SyncSession(SyncSessionContext* context, Delegate* delegate,
       delegate_(delegate),
       workers_(workers),
       routing_info_(routing_info),
-      enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)) {
+      enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)),
+      finished_(false) {
   status_controller_.reset(new StatusController(routing_info_));
   std::sort(workers_.begin(), workers_.end());
 }
@@ -127,6 +128,7 @@ void SyncSession::RebaseRoutingInfoWithLatest(const SyncSession& session) {
 }
 
 void SyncSession::PrepareForAnotherSyncCycle() {
+  finished_ = false;
   source_.updates_source =
       sync_pb::GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION;
   status_controller_.reset(new StatusController(routing_info_));
@@ -226,21 +228,41 @@ namespace {
 // successfully.
 //
 bool IsError(SyncerError error) {
-  return error != UNSET
-      && error != SYNCER_OK;
+  return error != UNSET && error != SYNCER_OK;
 }
-}  // namespace
 
-bool SyncSession::Succeeded() const {
+// Returns false iff one of the command results had an error.
+bool HadErrors(const ErrorCounters& error) {
   const bool download_updates_error =
-      IsError(status_controller_->error().last_download_updates_result);
-  const bool post_commit_error =
-      IsError(status_controller_->error().last_post_commit_result);
+      IsError(error.last_download_updates_result);
+  const bool post_commit_error = IsError(error.last_post_commit_result);
   const bool process_commit_response_error =
-      IsError(status_controller_->error().last_process_commit_response_result);
-  return !download_updates_error
-      && !post_commit_error
-      && !process_commit_response_error;
+      IsError(error.last_process_commit_response_result);
+  return download_updates_error ||
+         post_commit_error ||
+         process_commit_response_error;
+}
+}  // namespace
+
+bool SyncSession::Succeeded() const {
+  const ErrorCounters& error = status_controller_->error();
+  return finished_ && !HadErrors(error);
+}
+
+bool SyncSession::SuccessfullyReachedServer() const {
+  const ErrorCounters& error = status_controller_->error();
+  bool reached_server = error.last_download_updates_result == SYNCER_OK ||
+                        error.last_post_commit_result == SYNCER_OK ||
+                        error.last_process_commit_response_result == SYNCER_OK;
+  // It's possible that we reached the server on one attempt, then had an error
+  // on the next (or didn't perform some of the server-communicating commands).
+  // We want to verify that, for all commands attempted, we successfully spoke
+  // with the server. Therefore, we verify no errors and at least one SYNCER_OK.
+  return reached_server && !HadErrors(error);
+}
+
+void SyncSession::SetFinished() {
+  finished_ = true;
 }
 
 }  // namespace sessions
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index 94148d1..8b15d35 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -113,7 +113,7 @@ class SyncSession {
   // engine again.
   bool HasMoreToSync() const;
 
-  // Returns true if there we did not detect any errors in this session.
+  // Returns true if we completely ran the session without errors.
   //
   // There are many errors that could prevent a sync cycle from succeeding.
   // These include invalid local state, inability to contact the server,
@@ -129,6 +129,10 @@ class SyncSession {
   // since the last call to SyncShare.
   bool Succeeded() const;
 
+  // Returns true if we reached the server successfully and the server did not
+  // return any error codes. Returns false if no connection was attempted.
+  bool SuccessfullyReachedServer() const;
+
   // Collects all state pertaining to how and why |s| originated and unions it
   // with corresponding state in |this|, leaving |s| unchanged.  Allows |this|
   // to take on the responsibilities |s| had (e.g. certain data types) in the
@@ -177,6 +181,9 @@ class SyncSession {
   // Returns the set of enabled groups that have verified updates.
   std::set<ModelSafeGroup> GetEnabledGroupsWithVerifiedUpdates() const;
 
+  // Mark the session has having finished all the sync steps it needed.
+  void SetFinished();
+
  private:
   // Extend the encapsulation boundary to utilities for internal member
   // assignments. This way, the scope of these actions is explicit, they can't
@@ -214,6 +221,10 @@ class SyncSession {
   // |routing_info_|.
   std::set<ModelSafeGroup> enabled_groups_;
 
+  // Whether this session has reached its last step or not. Gets reset on each
+  // new cycle (via PrepareForAnotherSyncCycle).
+  bool finished_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncSession);
 };
 
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 0b1ad24..701072e 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -38,6 +38,15 @@ void SimulateSuccess(sessions::SyncSession* session,
     ADD_FAILURE() << "Shouldn't have more to sync";
   }
   ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining());
+  session->SetFinished();
+  if (end == SYNCER_END) {
+    session->mutable_status_controller()->set_last_download_updates_result(
+        SYNCER_OK);
+    session->mutable_status_controller()->set_last_post_commit_result(
+        SYNCER_OK);
+    session->mutable_status_controller()->
+        set_last_process_commit_response_result(SYNCER_OK);
+  }
 }
 
 void SimulateThrottledImpl(sessions::SyncSession* session,