sync: Retry soon when server returns conflict

When the server gives us a conflict response, we ought to fetch updates
in order to figure out what the server is complaining about, resolve any
conflicts locally, then re-commit.

The current syncer, although not intentionally built to handle this
scenario, does the right thing.  It considers the server's return code
to be indicative of a transient error, so it backs off then retries
later.  The retry sync cycle will fetch updates, resolve conflicts, and
recommit, which is exactly what we want.  Unfortunately, the backoff can
take five minutes.

This commit reduces the time spent unnecessarily backing off.  It's not
the cleanest solution, but its implementation is easy and safe.

BUG=157955


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

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

sync/engine/backoff_delay_provider.cc

@@ -97,6 +97,18 @@ TimeDelta BackoffDelayProvider::GetInitialDelay(
     return short_initial_backoff_;
   }
 
+  // When the server tells us we have a conflict, then we should download the
+  // latest updates so we can see the conflict ourselves, resolve it locally,
+  // then try again to commit.  Running another sync cycle will do all these
+  // things.  There's no need to back off, we can do this immediately.
+  //
+  // TODO(sync): We shouldn't need to handle this in BackoffDelayProvider.
+  // There should be a way to deal with protocol errors before we get to this
+  // point.
+  if (state.commit_result == SERVER_RETURN_CONFLICT) {
+    return short_initial_backoff_;
+  }
+
   return default_initial_backoff_;
 }
 

sync/engine/backoff_delay_provider_unittest.cc

@@ -72,6 +72,10 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
   state.commit_result = NETWORK_CONNECTION_UNAVAILABLE;
   EXPECT_EQ(kInitialBackoffImmediateRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
+
+  state.commit_result = SERVER_RETURN_CONFLICT;
+  EXPECT_EQ(kInitialBackoffImmediateRetrySeconds,
+            delay->GetInitialDelay(state).InSeconds());
 }
 
 TEST_F(BackoffDelayProviderTest, GetInitialDelayWithOverride) {
@@ -106,6 +110,10 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelayWithOverride) {
   state.commit_result = SERVER_RETURN_MIGRATION_DONE;
   EXPECT_EQ(kInitialBackoffImmediateRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
+
+  state.commit_result = SERVER_RETURN_CONFLICT;
+  EXPECT_EQ(kInitialBackoffImmediateRetrySeconds,
+            delay->GetInitialDelay(state).InSeconds());
 }
 
 }  // namespace syncer

sync/engine/process_commit_response_command.cc

@@ -171,21 +171,9 @@ SyncerError ProcessCommitResponseCommand::ProcessCommitResponse(
     // to resolve the conflict.  That's not what this client does.
     //
     // We don't currently have any code to support that exceptional control
-    // flow.  We don't intend to add any because this response code will be
-    // deprecated soon.  Instead, we handle this in the same way that we handle
-    // transient errors.  We abort the current sync cycle, wait a little while,
-    // then try again.  The retry sync cycle will attempt to download updates
-    // which should be sufficient to trigger client-side conflict resolution.
-    //
-    // Not treating this as an error would be dangerous.  There's a risk that
-    // the commit loop would loop indefinitely.  The loop won't exit until the
-    // number of unsynced items hits zero or an error is detected.  If we're
-    // constantly receiving conflict responses and we don't treat them as
-    // errors, there would be no reason to leave that loop.
-    //
-    // TODO: Remove this option when the CONFLICT return value is fully
-    // deprecated.
-    return SERVER_RETURN_TRANSIENT_ERROR;
+    // flow.  Instead, we abort the current sync cycle and start a new one.  The
+    // end result is the same.
+    return SERVER_RETURN_CONFLICT;
   } else {
     LOG(FATAL) << "Inconsistent counts when processing commit response";
     return SYNCER_OK;

sync/internal_api/public/util/syncer_error.cc

@@ -12,7 +12,6 @@ namespace syncer {
 const char* GetSyncerErrorString(SyncerError value) {
   switch (value) {
     ENUM_CASE(UNSET);
-    ENUM_CASE(DIRECTORY_LOOKUP_FAILED);
     ENUM_CASE(CANNOT_DO_WORK);
     ENUM_CASE(NETWORK_CONNECTION_UNAVAILABLE);
     ENUM_CASE(NETWORK_IO_ERROR);
@@ -25,6 +24,7 @@ const char* GetSyncerErrorString(SyncerError value) {
     ENUM_CASE(SERVER_RETURN_MIGRATION_DONE);
     ENUM_CASE(SERVER_RETURN_CLEAR_PENDING);
     ENUM_CASE(SERVER_RETURN_NOT_MY_BIRTHDAY);
+    ENUM_CASE(SERVER_RETURN_CONFLICT);
     ENUM_CASE(SERVER_RESPONSE_VALIDATION_FAILED);
     ENUM_CASE(SYNCER_OK);
   }

sync/internal_api/public/util/syncer_error.h

@@ -18,7 +18,6 @@ namespace syncer {
 // it makes refactoring easier.
 enum SyncerError {
   UNSET = 0,       // Default value.
-  DIRECTORY_LOOKUP_FAILED,  // Local directory lookup failure.
   CANNOT_DO_WORK,  // A model worker could not process a work item.
 
   NETWORK_CONNECTION_UNAVAILABLE,  // Connectivity failure.
@@ -36,6 +35,7 @@ enum SyncerError {
   SERVER_RETURN_MIGRATION_DONE,
   SERVER_RETURN_CLEAR_PENDING,
   SERVER_RETURN_NOT_MY_BIRTHDAY,
+  SERVER_RETURN_CONFLICT,
   SERVER_RESPONSE_VALIDATION_FAILED,
 
   SYNCER_OK