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

sync/sessions/sync_session.cc

Index: sync/sessions/sync_session.cc
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 85ac8ccd7b72b6785191bc3915ae544f96420bd4..ecea5b161205509906c2d1d731219a2bfb995021 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());
 }
@@ -226,21 +227,39 @@ 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 reach_server = false;

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

Rather than use an if statement, why not assign the result directly to the bool?
 

const bool reached_server =
  (error.last_download_updates_result == SYNCER_OK ||
   ...);

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

Done.

+  if (error.last_download_updates_result == SYNCER_OK ||
+      error.last_post_commit_result == SYNCER_OK ||
+      error.last_process_commit_response_result == SYNCER_OK)
+    reach_server = true;
+  return reach_server && !HadErrors(error);

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

This is trickier than it looks.  You got it right, but it probably deserves a
comment somewhere.  

We can't allow backoff relief based on the 'reach_server' condition alone.  That
would cause us to behave badly if the server was rejecting commits but accepting
GU requests.  

Could you add a comment explaining why it's necessary to check HadErrors here?

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

Done.

+}
+
+void SyncSession::SetFinished() {
+  finished_ = true;
 }
 
 }  // namespace sessions