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

sync/sessions/sync_session.h

Index: sync/sessions/sync_session.h
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index 1dfe78cfd1cb3fe20577360aa8b35e014a29a936..95aa822bf8a5d79474dfb0ee58bbe25c9c974e8c 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -112,7 +112,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,
@@ -126,8 +126,14 @@ class SyncSession {
   // This function also returns false if SyncShare has not been called on this
   // session yet, or if ResetTransientState() has been called on this session
   // since the last call to SyncShare.
+  //
+  // Note: this method ignore Sync Protocol and Client Actionable errors.

[rlarocque, 2012/06/06 21:12:05]

I don't think that's true.  Any response other than SYNCER_OK should be an
error.

Also, grammar nit: s/ignore/ignores/.

[Nicolas Zea, 2012/06/06 21:27:55]

Protocol and actionable errors are not reflected in the SyncerError's for the
three commands, but in the protocol error and actionable error members of
ErrorCounter, which we're not checking here.

Protocol errors are handled by invoking OnSyncProtocolErrors, and actionable
errors are handled via OnActionableError.

I was tempted to check them as well, but if that were the case we would wind up
retrying on migration and other errors as well, which I believe you recently had
to fix.

   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
@@ -176,6 +182,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
@@ -213,6 +222,9 @@ class SyncSession {
   // |routing_info_|.
   std::set<ModelSafeGroup> enabled_groups_;
 
+  // Whether this session has reached its last step or not.
+  bool finished_;

[rlarocque, 2012/06/06 20:51:22]

If the session requires multiple calls to SyncShare() (because of
HasMoreToSync()), we might end up reaching the "last step" more than once.

Maybe this should be set only when we leave the while loop in
SyncScheduler::DoSyncSessionJob()?

[Nicolas Zea, 2012/06/06 21:27:55]

Good catch.  I reset the flag in PrepareForAnotherSyncCycle now, as I'd still
rather ensure we completed the last cycle successfully.

+
   DISALLOW_COPY_AND_ASSIGN(SyncSession);
 };