Abort sync cycles when download step fails

sync/engine/syncer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/syncer.h"
 
 #include "base/debug/trace_event.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
@@ skipping 92 matching lines @@
   SyncerStep current_step = first_step;
 
   SyncerStep next_step = current_step;
   while (!ExitRequested()) {
     TRACE_EVENT1("sync", "SyncerStateMachine",
                  "state", SyncerStepToString(current_step));
     DVLOG(1) << "Syncer step:" << SyncerStepToString(current_step);
 
     switch (current_step) {
       case SYNCER_BEGIN:
-        // This isn't perfect, as we can end up bundling extensions activity
-        // intended for the next session into the current one.  We could do a
-        // test-and-reset as with the source, but note that also falls short if
-        // the commit request fails (e.g. due to lost connection), as we will
-        // fall all the way back to the syncer thread main loop in that case,
-        // creating a new session when a connection is established, losing the
-        // records set here on the original attempt.  This should provide us
-        // with the right data "most of the time", and we're only using this
-        // for analysis purposes, so Law of Large Numbers FTW.
-        session->context()->extensions_monitor()->GetAndClearRecords(
-            session->mutable_extensions_activity());
         session->context()->PruneUnthrottledTypes(base::TimeTicks::Now());
         session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
 
         next_step = CLEANUP_DISABLED_TYPES;
         break;
       case CLEANUP_DISABLED_TYPES: {
         CleanupDisabledTypesCommand cleanup;
         cleanup.Execute(session);
         next_step = DOWNLOAD_UPDATES;
         break;
@@ skipping 25 matching lines @@
       }
       case PROCESS_UPDATES: {
         ProcessUpdatesCommand process_updates;
         process_updates.Execute(session);
         next_step = STORE_TIMESTAMPS;
         break;
       }
       case STORE_TIMESTAMPS: {
         StoreTimestampsCommand store_timestamps;
         store_timestamps.Execute(session);
-        // We should download all of the updates before attempting to process
-        // them.
-        if (session->status_controller().ServerSaysNothingMoreToDownload() ||
-            !session->status_controller().download_updates_succeeded()) {
+        // We download all of the updates before attempting to apply them.
+        SyncerError result =
+            session->status_controller().last_download_updates_result();
+        if (result == SYNCER_OK) {
+          const ClientToServerResponse& updates_response =
+              session->status_controller().updates_response();

[tim (not reviewing), 2012/04/20 16:41:07]

You're doing this just to get changes_remaining, right? StoreTimestamps already
process this field and stores it in the status controller right now. Syncer
shouldn't be privy to full raw proto message types.

I'm surprised you didn't have to add an include for ClientToServerResponse, but
it looks like it's because of ProcessClientCommand still living in here. 

[rlarocque, 2012/04/23 20:16:49]

The next revision of this patch should avoid this issue.

+          // Loop back if we still have more to download.  Otherwise exit this
+          // loop normally.
+          DCHECK(updates_response.get_updates().has_changes_remaining());
+          if (updates_response.get_updates().changes_remaining() != 0) {
+            next_step = DOWNLOAD_UPDATES;
+          } else {
+            next_step = APPLY_UPDATES;
+          }
+        } else if (result == SERVER_RETURN_MIGRATION_DONE)  {
+          // The MIGRATION_DONE case is special.  The migration test code relies

[tim (not reviewing), 2012/04/20 16:41:07]

As far as I can tell it is actually handled naturally by the block above, since
changes_remaining *must* be zero if the server sends migration done. If it's not
we should set it to zero, because whatever the number is it cannot reflect the
state in our progress markers by definition!  I may be missing something, but it
seems like we're making things more complicated with an unnecessary branch for
the same benefit we could get by simply tweaking your DCHECK on 166 to have an
|| for return_migration_done, alongside your nice comment.

[rlarocque, 2012/04/23 20:16:49]

Same as above.

+          // on all sorts of strange side effects, and this is one of them.  The
+          // effect of this special case is that we will count downloads that
+          // encounter migration_done errors as non-failures, while jobs that do
+          // attempt to commit will be counted as failures most of the time,

[tim (not reviewing), 2012/04/20 16:41:07]

I'm having a hard time groking this: "... we will count downloads that encounter
MD errors as non-failures, while jobs that do attempt to commit..." - something
feels apples-to-oranges about that statement. Can you reword it? 

[rlarocque, 2012/04/23 20:16:49]

This will also be simplified in the next revision.  

+          // because they will fail PROCESS_COMMIT_RESPONSE.  In short, we're
+          // trying to make sure that the necessary changes for crbug.com/122033
+          // do not upset the delicate balance that allows the migration tests
+          // to pass.
+          //
+          // We are tracking some of the effort to clean up this mess at
+          // crbug.com/123954.
           next_step = APPLY_UPDATES;
         } else {
-          next_step = DOWNLOAD_UPDATES;
+          // All other errors.  We should not attempt to apply updates until we
+          // have received all of them.  At this point, we have not received all
+          // downloads so we should not attempt to apply any of the updates that
+          // we have downloded successfully.
+          last_step = SYNCER_END; // Necessary for CONFIGURATION mode.
+          next_step = SYNCER_END;
+          DVLOG(1) << "Aborting sync cycle due to download updates failure";
         }
         break;
       }
       case APPLY_UPDATES: {
         ApplyUpdatesCommand apply_updates;
         apply_updates.Execute(session);
         if (last_step == APPLY_UPDATES) {
           // We're in configuration mode, but we still need to run the
           // SYNCER_END step.
           last_step = SYNCER_END;
@@ skipping 10 matching lines @@
         WriteTransaction trans(FROM_HERE, SYNCER, dir);
         sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
 
         DVLOG(1) << "Getting the Commit IDs";
         GetCommitIdsCommand get_commit_ids_command(
             session->context()->max_commit_batch_size());
         get_commit_ids_command.Execute(session);
 
         if (!session->status_controller().commit_ids().empty()) {
           DVLOG(1) << "Building a commit message";
+
+          // This isn't perfect, since the set of extensions activity may not
+          // correlate exactly with the items being committed.  That's OK as
+          // long as we're looking for a rough estimate of extensions activity,
+          // not an precise mapping of which commits were triggered by which
+          // extension.
+          //
+          // We will push this list of extensions activity back into the
+          // ExtensionsActivityMonitor if this commit turns out to not contain
+          // any bookmarks, or if the commit fails.
+          session->context()->extensions_monitor()->GetAndClearRecords(
+              session->mutable_extensions_activity());
+
           BuildCommitCommand build_commit_command;
           build_commit_command.Execute(session);
 
           next_step = POST_COMMIT_MESSAGE;
         } else {
           next_step = RESOLVE_CONFLICTS;
         }
 
         break;
       }
@@ skipping 120 matching lines @@
   entry->Put(SERVER_CTIME, Time());
   entry->Put(SERVER_VERSION, 0);
   entry->Put(SERVER_IS_DIR, false);
   entry->Put(SERVER_IS_DEL, false);
   entry->Put(IS_UNAPPLIED_UPDATE, false);
   entry->Put(SERVER_SPECIFICS, sync_pb::EntitySpecifics::default_instance());
   entry->Put(SERVER_POSITION_IN_PARENT, 0);
 }
 
 }  // namespace browser_sync