sync: GU retry with less explicit TimeTicks logic

sync/sessions/nudge_tracker_unittest.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 "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "sync/internal_api/public/base/model_type_test_util.h"
 #include "sync/notifier/invalidation_util.h"
 #include "sync/notifier/mock_ack_handler.h"
 #include "sync/notifier/object_id_invalidation_map.h"
@@ skipping 51 matching lines @@
   }
 
   int ProtoRefreshRequestedCount(ModelType type) const {
     sync_pb::GetUpdateTriggers gu_trigger;
     nudge_tracker_.FillProtoMessage(type, &gu_trigger);
     return gu_trigger.datatype_refresh_nudges();
   }
 
   void SetInvalidationsInSync() {
     nudge_tracker_.OnInvalidationsEnabled();
-    nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+    nudge_tracker_.RecordSuccessfulSyncCycle();
   }
 
  protected:
   NudgeTracker nudge_tracker_;
 };
 
 // Exercise an empty NudgeTracker.
 // Use with valgrind to detect uninitialized members.
 TEST_F(NudgeTrackerTest, EmptyNudgeTracker) {
   // Now we're at the normal, "idle" state.
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::UNKNOWN,
             nudge_tracker_.updates_source());
 
   sync_pb::GetUpdateTriggers gu_trigger;
   nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger);
 
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::UNKNOWN,
             nudge_tracker_.updates_source());
 }
 
@@ skipping 169 matching lines @@
   nudge_tracker_.RecordRemoteInvalidation(invalidation_map);
   {
     sync_pb::GetUpdateTriggers gu_trigger;
     nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger);
     EXPECT_TRUE(gu_trigger.server_dropped_hints());
     EXPECT_FALSE(gu_trigger.client_dropped_hints());
     ASSERT_EQ(0, gu_trigger.notification_hint_size());
   }
 
   // Clear status then verify.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   {
     sync_pb::GetUpdateTriggers gu_trigger;
     nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger);
     EXPECT_FALSE(gu_trigger.client_dropped_hints());
     EXPECT_FALSE(gu_trigger.server_dropped_hints());
     ASSERT_EQ(0, gu_trigger.notification_hint_size());
   }
 }
 
 // Tests the receipt of 'unknown version' invalidations.  This test also
 // includes a known version invalidation to mix things up a bit.
 TEST_F(NudgeTrackerTest, DropHintsAtServer_WithOtherInvalidations) {
   ObjectIdInvalidationMap invalidation_map;
   invalidation_map.Insert(BuildUnknownVersionInvalidation(BOOKMARKS));
   invalidation_map.Insert(BuildInvalidation(BOOKMARKS, 10, "hint"));
 
   // Record the two invalidations, one with unknown version, the other unknown.
   nudge_tracker_.RecordRemoteInvalidation(invalidation_map);
   {
     sync_pb::GetUpdateTriggers gu_trigger;
     nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger);
     EXPECT_TRUE(gu_trigger.server_dropped_hints());
     EXPECT_FALSE(gu_trigger.client_dropped_hints());
     ASSERT_EQ(1, gu_trigger.notification_hint_size());
     EXPECT_EQ("hint", gu_trigger.notification_hint(0));
   }
 
   // Clear status then verify.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   {
     sync_pb::GetUpdateTriggers gu_trigger;
     nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger);
     EXPECT_FALSE(gu_trigger.client_dropped_hints());
     EXPECT_FALSE(gu_trigger.server_dropped_hints());
     ASSERT_EQ(0, gu_trigger.notification_hint_size());
   }
 }
 
 // Checks the behaviour of the invalidations-out-of-sync flag.
 TEST_F(NudgeTrackerTest, EnableDisableInvalidations) {
   // Start with invalidations offline.
   nudge_tracker_.OnInvalidationsDisabled();
   EXPECT_TRUE(InvalidationsOutOfSync());
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 
   // Simply enabling invalidations does not bring us back into sync.
   nudge_tracker_.OnInvalidationsEnabled();
   EXPECT_TRUE(InvalidationsOutOfSync());
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 
   // We must successfully complete a sync cycle while invalidations are enabled
   // to be sure that we're in sync.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_FALSE(InvalidationsOutOfSync());
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // If the invalidator malfunctions, we go become unsynced again.
   nudge_tracker_.OnInvalidationsDisabled();
   EXPECT_TRUE(InvalidationsOutOfSync());
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 
   // A sync cycle while invalidations are disabled won't reset the flag.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_TRUE(InvalidationsOutOfSync());
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 
   // Nor will the re-enabling of invalidations be sufficient, even now that
   // we've had a successful sync cycle.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_TRUE(InvalidationsOutOfSync());
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 }
 
 // Tests that locally modified types are correctly written out to the
 // GetUpdateTriggers proto.
 TEST_F(NudgeTrackerTest, WriteLocallyModifiedTypesToProto) {
   // Should not be locally modified by default.
   EXPECT_EQ(0, ProtoLocallyModifiedCount(PREFERENCES));
 
   // Record a local bookmark change.  Verify it was registered correctly.
   nudge_tracker_.RecordLocalChange(ModelTypeSet(PREFERENCES));
   EXPECT_EQ(1, ProtoLocallyModifiedCount(PREFERENCES));
 
   // Record a successful sync cycle.  Verify the count is cleared.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_EQ(0, ProtoLocallyModifiedCount(PREFERENCES));
 }
 
 // Tests that refresh requested types are correctly written out to the
 // GetUpdateTriggers proto.
 TEST_F(NudgeTrackerTest, WriteRefreshRequestedTypesToProto) {
   // There should be no refresh requested by default.
   EXPECT_EQ(0, ProtoRefreshRequestedCount(SESSIONS));
 
   // Record a local refresh request.  Verify it was registered correctly.
   nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS));
   EXPECT_EQ(1, ProtoRefreshRequestedCount(SESSIONS));
 
   // Record a successful sync cycle.  Verify the count is cleared.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_EQ(0, ProtoRefreshRequestedCount(SESSIONS));
 }
 
 // Basic tests for the IsSyncRequired() flag.
 TEST_F(NudgeTrackerTest, IsSyncRequired) {
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
   // Local changes.
   nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS));
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
   // Refresh requests.
   nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS));
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
   // Invalidations.
   ObjectIdInvalidationMap invalidation_map =
       BuildInvalidationMap(PREFERENCES, 1, "hint");
   nudge_tracker_.RecordRemoteInvalidation(invalidation_map);
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 }
 
 // Basic tests for the IsGetUpdatesRequired() flag.
 TEST_F(NudgeTrackerTest, IsGetUpdatesRequired) {
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // Local changes.
   nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS));
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // Refresh requests.
   nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS));
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // Invalidations.
   ObjectIdInvalidationMap invalidation_map =
       BuildInvalidationMap(PREFERENCES, 1, "hint");
   nudge_tracker_.RecordRemoteInvalidation(invalidation_map);
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 }
 
 // Test IsSyncRequired() responds correctly to data type throttling.
 TEST_F(NudgeTrackerTest, IsSyncRequired_Throttling) {
   const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(1234);
   const base::TimeDelta throttle_length = base::TimeDelta::FromMinutes(10);
   const base::TimeTicks t1 = t0 + throttle_length;
 
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
   // A local change to sessions enables the flag.
   nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS));
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
 
   // But the throttling of sessions unsets it.
   nudge_tracker_.SetTypesThrottledUntil(ModelTypeSet(SESSIONS),
                                        throttle_length,
                                        t0);
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
   // A refresh request for bookmarks means we have reason to sync again.
   nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(BOOKMARKS));
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
 
   // A successful sync cycle means we took care of bookmarks.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
   // But we still haven't dealt with sessions.  We'll need to remember
   // that sessions are out of sync and re-enable the flag when their
   // throttling interval expires.
   nudge_tracker_.UpdateTypeThrottlingState(t1);
   EXPECT_FALSE(nudge_tracker_.IsTypeThrottled(SESSIONS));
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
 }
 
 // Test IsGetUpdatesRequired() responds correctly to data type throttling.
 TEST_F(NudgeTrackerTest, IsGetUpdatesRequired_Throttling) {
   const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(1234);
   const base::TimeDelta throttle_length = base::TimeDelta::FromMinutes(10);
   const base::TimeTicks t1 = t0 + throttle_length;
 
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // A refresh request to sessions enables the flag.
   nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS));
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 
   // But the throttling of sessions unsets it.
   nudge_tracker_.SetTypesThrottledUntil(ModelTypeSet(SESSIONS),
                                        throttle_length,
                                        t0);
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // A refresh request for bookmarks means we have reason to sync again.
   nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(BOOKMARKS));
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 
   // A successful sync cycle means we took care of bookmarks.
-  nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
 
   // But we still haven't dealt with sessions.  We'll need to remember
   // that sessions are out of sync and re-enable the flag when their
   // throttling interval expires.
   nudge_tracker_.UpdateTypeThrottlingState(t1);
   EXPECT_FALSE(nudge_tracker_.IsTypeThrottled(SESSIONS));
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now()));
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
 }
 
 // Tests throttling-related getter functions when no types are throttled.
 TEST_F(NudgeTrackerTest, NoTypesThrottled) {
   EXPECT_FALSE(nudge_tracker_.IsAnyTypeThrottled());
   EXPECT_FALSE(nudge_tracker_.IsTypeThrottled(SESSIONS));
   EXPECT_TRUE(nudge_tracker_.GetThrottledTypes().Empty());
 }
 
 // Tests throttling-related getter functions when some types are throttled.
@@ skipping 56 matching lines @@
           nudge_tracker_.GetThrottledTypes()));
   EXPECT_EQ(throttle2_length - throttle1_length,
             nudge_tracker_.GetTimeUntilNextUnthrottle(t1));
 
   // Expire the second interval.
   nudge_tracker_.UpdateTypeThrottlingState(t2);
   EXPECT_TRUE(nudge_tracker_.GetThrottledTypes().Empty());
 }
 
 TEST_F(NudgeTrackerTest, Retry) {
-  const base::TimeTicks retry_time = base::TimeTicks::FromInternalValue(12345);
-  const base::TimeTicks pre_retry_time =
-      retry_time - base::TimeDelta::FromSeconds(1);
-  const base::TimeTicks post_retry_time =
-      retry_time + base::TimeDelta::FromSeconds(1);
-  nudge_tracker_.set_next_retry_time(retry_time);
+  const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(12345);
+  const base::TimeTicks t3 = t0 + base::TimeDelta::FromSeconds(3);
+  const base::TimeTicks t4 = t0 + base::TimeDelta::FromSeconds(4);
 
-  EXPECT_FALSE(nudge_tracker_.IsRetryRequired(pre_retry_time));
-  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(pre_retry_time));
-  nudge_tracker_.RecordSuccessfulSyncCycle(pre_retry_time);
+  // Set retry for t3.
+  nudge_tracker_.SetNextRetryTime(t3);
 
-  EXPECT_TRUE(nudge_tracker_.IsRetryRequired(post_retry_time));
-  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(post_retry_time));
-  nudge_tracker_.RecordSuccessfulSyncCycle(post_retry_time);
-  EXPECT_FALSE(nudge_tracker_.IsRetryRequired(
-      post_retry_time + base::TimeDelta::FromSeconds(1)));
+  // Not due yet at t0.
+  nudge_tracker_.SetSyncCycleStartTime(t0);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+
+  // Successful sync cycle at t0 changes nothing.
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+
+  // At t4, the retry becomes due.
+  nudge_tracker_.SetSyncCycleStartTime(t4);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
+
+  // A sync cycle unsets the flag.
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+
+  // It's still unset at the start of the next sync cycle.
+  nudge_tracker_.SetSyncCycleStartTime(t4);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+}
+
+// Test a mid-cycle update when IsRetryRequired() was true before the cycle
+// began.
+TEST_F(NudgeTrackerTest, IsRetryRequired_MidCycleUpdate1) {
+  const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(12345);
+  const base::TimeTicks t1 = t0 + base::TimeDelta::FromSeconds(1);
+  const base::TimeTicks t2 = t0 + base::TimeDelta::FromSeconds(2);
+  const base::TimeTicks t5 = t0 + base::TimeDelta::FromSeconds(5);
+  const base::TimeTicks t6 = t0 + base::TimeDelta::FromSeconds(6);
+
+  nudge_tracker_.SetNextRetryTime(t0);
+  nudge_tracker_.SetSyncCycleStartTime(t1);
+
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // Pretend that we were updated mid-cycle.  SetSyncCycleStartTime is
+  // called only at the start of the sync cycle, so don't call it here.
+  // The update should have no effect on IsRetryRequired().
+  nudge_tracker_.SetNextRetryTime(t5);
+
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // Verify that the successful sync cycle clears the flag.
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+
+  // Verify expecations around the new retry time.
+  nudge_tracker_.SetSyncCycleStartTime(t2);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+
+  nudge_tracker_.SetSyncCycleStartTime(t6);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+}
+
+// Test a mid-cycle update when IsRetryRequired() was false before the cycle
+// began.
+TEST_F(NudgeTrackerTest, IsRetryRequired_MidCycleUpdate2) {
+  const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(12345);
+  const base::TimeTicks t1 = t0 + base::TimeDelta::FromSeconds(1);
+  const base::TimeTicks t3 = t0 + base::TimeDelta::FromSeconds(3);
+  const base::TimeTicks t5 = t0 + base::TimeDelta::FromSeconds(5);
+  const base::TimeTicks t6 = t0 + base::TimeDelta::FromSeconds(6);
+
+  // Schedule a future retry, and a nudge unrelated to it.
+  nudge_tracker_.RecordLocalChange(ModelTypeSet(BOOKMARKS));
+  nudge_tracker_.SetNextRetryTime(t1);
+  nudge_tracker_.SetSyncCycleStartTime(t0);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+
+  // Pretend this happened in mid-cycle.  This should have no effect on
+  // IsRetryRequired().
+  nudge_tracker_.SetNextRetryTime(t5);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+
+  // The cycle succeeded.
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+
+  // The time t3 is greater than the GU retry time scheduled at the beginning of
+  // the test, but later than the retry time that overwrote it during the
+  // pretend 'sync cycle'.
+  nudge_tracker_.SetSyncCycleStartTime(t3);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+
+  // Finally, the retry established during the sync cycle becomes due.
+  nudge_tracker_.SetSyncCycleStartTime(t6);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+}
+
+// Simulate the case where a sync cycle fails.
+TEST_F(NudgeTrackerTest, IsRetryRequired_FailedCycle) {
+  const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(12345);
+  const base::TimeTicks t1 = t0 + base::TimeDelta::FromSeconds(1);
+  const base::TimeTicks t2 = t0 + base::TimeDelta::FromSeconds(2);
+
+  nudge_tracker_.SetNextRetryTime(t0);
+  nudge_tracker_.SetSyncCycleStartTime(t1);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // The nudge tracker receives no notifications for a failed sync cycle.
+  // Pretend one happened here.
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // Think of this as the retry cycle.
+  nudge_tracker_.SetSyncCycleStartTime(t2);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // The second cycle is a success.
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+}
+
+// Simulate a partially failed sync cycle.  The callback to update the GU retry
+// was invoked, but the sync cycle did not complete successfully.
+TEST_F(NudgeTrackerTest, IsRetryRequired_FailedCycleIncludesUpdate) {
+  const base::TimeTicks t0 = base::TimeTicks::FromInternalValue(12345);
+  const base::TimeTicks t1 = t0 + base::TimeDelta::FromSeconds(1);
+  const base::TimeTicks t3 = t0 + base::TimeDelta::FromSeconds(3);
+  const base::TimeTicks t4 = t0 + base::TimeDelta::FromSeconds(4);
+  const base::TimeTicks t5 = t0 + base::TimeDelta::FromSeconds(5);
+  const base::TimeTicks t6 = t0 + base::TimeDelta::FromSeconds(6);
+
+  nudge_tracker_.SetNextRetryTime(t0);
+  nudge_tracker_.SetSyncCycleStartTime(t1);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // The cycle is in progress.  A new GU Retry time is received.
+  // The flag is not because this cycle is still in progress.
+  nudge_tracker_.SetNextRetryTime(t5);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // The nudge tracker receives no notifications for a failed sync cycle.
+  // Pretend the cycle failed here.
+
+  // The next sync cycle starts.  The new GU time has not taken effect by this
+  // time, but the NudgeTracker hasn't forgotten that we have not yet serviced
+  // the retry from the previous cycle.
+  nudge_tracker_.SetSyncCycleStartTime(t3);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+
+  // It succeeds.  The retry time is not updated, so it should remain at t5.
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+
+  // Another sync cycle.  This one is still before the scheduled retry.  It does
+  // not change the scheduled retry time.
+  nudge_tracker_.SetSyncCycleStartTime(t4);
+  EXPECT_FALSE(nudge_tracker_.IsRetryRequired());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+
+  // The retry scheduled way back during the first cycle of this test finally
+  // becomes due.  Perform a successful sync cycle to service it.
+  nudge_tracker_.SetSyncCycleStartTime(t6);
+  EXPECT_TRUE(nudge_tracker_.IsRetryRequired());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
 }
 
 class NudgeTrackerAckTrackingTest : public NudgeTrackerTest {
  public:
   NudgeTrackerAckTrackingTest() {}
 
   bool IsInvalidationUnacknowledged(const syncer::Invalidation& invalidation) {
     // Run pending tasks before checking with the MockAckHandler.
     // The WeakHandle may have posted some tasks for it.
     base::RunLoop().RunUntilIdle();
@@ skipping 43 matching lines @@
     // Send it to the NudgeTracker.
     ObjectIdInvalidationMap invalidation_map;
     invalidation_map.Insert(invalidation);
     nudge_tracker_.RecordRemoteInvalidation(invalidation_map);
 
     // Return it to the test framework for use in assertions.
     return invalidation;
   }
 
   void RecordSuccessfulSyncCycle() {
-    nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+    nudge_tracker_.RecordSuccessfulSyncCycle();
   }
 
  private:
   syncer::MockAckHandler mock_ack_handler_;
   base::MessageLoop loop_;
 };
 
 // Test the acknowledgement of a single invalidation.
 TEST_F(NudgeTrackerAckTrackingTest, SimpleAcknowledgement) {
   Invalidation inv = SendInvalidation(BOOKMARKS, 10, "hint");
@@ skipping 92 matching lines @@
   EXPECT_TRUE(IsInvalidationAcknowledged(inv2));
   EXPECT_TRUE(IsInvalidationAcknowledged(inv3));
   EXPECT_TRUE(IsInvalidationAcknowledged(inv4));
   EXPECT_TRUE(IsInvalidationAcknowledged(inv5));
 
   EXPECT_TRUE(AllInvalidationsAccountedFor());
 }
 
 }  // namespace sessions
 }  // namespace syncer