Fix flakiness in TwoClientPrintersSyncTest.ConflictResolution

The hypothesis here is that the test outcome is, strictly speaking,
non-deterministic, because a server with an eventual consistency model
cannot guarantee how exactly a conflict will be resolved (although it
will guarantee that clients converge). In this model, the last commit
will win.

We fix the previously existing test by making sure which of the two
clients commits first to the server. This is achieved by mimic-ing an
offline state (or poor connectivity compared to the other client).

In order to verify the hypothesis, we add a second test that enables
strong consistency model on the server (implemented as part of this
patch), and hence relies on that mechanism to expect a deterministic
outcome without having to put clients offline.

Bug: 810408
Change-Id: I2fa3fd8c941674344030a76e76f7855131c1908e
Reviewed-on: https://chromium-review.googlesource.com/1249482
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594865}

chrome/browser/sync/test/integration/two_client_printers_sync_test.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "build/build_config.h"
 #include "chrome/browser/sync/test/integration/printers_helper.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "content/public/test/test_utils.h"
 
@@ -119,31 +120,79 @@ IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, ConflictResolution) {
   // Store 0 and 1 have 3 printers now.
   ASSERT_TRUE(PrintersMatchChecker().Wait());
 
+  // Client 1 makes a local change.
   ASSERT_TRUE(
       EditPrinterDescription(GetPrinterStore(1), 0, kOverwrittenDescription));
 
-  // Wait for a non-zero period (200ms).
+  // Wait for a non-zero period (200ms) for modification timestamps to differ.
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(200));
 
-  // Run all tasks so the first description change is applied.
-  // TODO(crbug.com/810408): This is a temporary fix to prevent flakiness. Tasks
-  // shouldn't run between description changes, because it prevents a real
-  // conflict from happening.
-  content::RunAllTasksUntilIdle();
+  // Client 0 goes offline, to make this test deterministic (client 1 commits
+  // first).
+  GetClient(0)->StopSyncService(syncer::SyncService::KEEP_DATA);
 
+  // Client 0 makes a change while offline.
   ASSERT_TRUE(
       EditPrinterDescription(GetPrinterStore(0), 0, kLatestDescription));
 
+  // We must wait until the sync cycle is completed before client 0 goes online
+  // in order to make the outcome of conflict resolution deterministic (needed
+  // due to lack of a strong consistency model on the server).
+  ProfileSyncServiceHarness::AwaitQuiescence({GetClient(1)});
+
+  ASSERT_EQ(GetPrinterStore(0)->GetConfiguredPrinters()[0].description(),
+            kLatestDescription);
+  ASSERT_EQ(GetPrinterStore(1)->GetConfiguredPrinters()[0].description(),
+            kOverwrittenDescription);
+
+  // Client 0 goes online, which results in a conflict (local wins).
+  GetClient(0)->StartSyncService();
+
   // Run tasks until the most recent update has been applied to all stores.
-  AwaitMatchStatusChangeChecker wait_latest_description_propagated(
-      base::BindRepeating([]() {
-        return GetPrinterStore(0)->GetConfiguredPrinters()[0].description() ==
-                   kLatestDescription &&
-               GetPrinterStore(1)->GetConfiguredPrinters()[0].description() ==
-                   kLatestDescription;
-      }),
-      "Description not propagated");
-  ASSERT_TRUE(wait_latest_description_propagated.Wait());
+  ASSERT_TRUE(PrintersMatchChecker().Wait());
+
+  EXPECT_EQ(GetPrinterStore(0)->GetConfiguredPrinters()[0].description(),
+            kLatestDescription);
+  EXPECT_EQ(GetPrinterStore(1)->GetConfiguredPrinters()[0].description(),
+            kLatestDescription);
+}
+
+IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest,
+                       ConflictResolutionWithStrongConsistency) {
+  ASSERT_TRUE(SetupSync());
+  GetFakeServer()->EnableStrongConsistencyWithConflictDetectionModel();
+
+  AddPrinter(GetPrinterStore(0), CreateTestPrinter(0));
+  AddPrinter(GetPrinterStore(0), CreateTestPrinter(2));
+  AddPrinter(GetPrinterStore(0), CreateTestPrinter(3));
+
+  // Store 0 and 1 have 3 printers now.
+  ASSERT_TRUE(PrintersMatchChecker().Wait());
+
+  // Client 1 makes a local change.
+  ASSERT_TRUE(
+      EditPrinterDescription(GetPrinterStore(1), 0, kOverwrittenDescription));
+
+  // Wait for a non-zero period (200ms) for modification timestamps to differ.
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(200));
+
+  // Client 0 makes a change to the same printer.
+  ASSERT_TRUE(
+      EditPrinterDescription(GetPrinterStore(0), 0, kLatestDescription));
+
+  // Run tasks until the most recent update has been applied to all stores.
+  // One of the two clients (the second one committing) will be requested by the
+  // server to resolve the conflict and recommit. The custom conflict resolution
+  // as implemented in PrintersSyncBridge::ResolveConflict() should guarantee
+  // that the one with latest modification timestamp (kLatestDescription) wins,
+  // which can mean local wins or remote wins, depending on which client is
+  // involved.
+  ASSERT_TRUE(PrintersMatchChecker().Wait());
+
+  EXPECT_EQ(GetPrinterStore(0)->GetConfiguredPrinters()[0].description(),
+            kLatestDescription);
+  EXPECT_EQ(GetPrinterStore(1)->GetConfiguredPrinters()[0].description(),
+            kLatestDescription);
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, SimpleMerge) {

components/sync/engine_impl/loopback_server/loopback_server.cc

@@ -133,7 +133,8 @@ class UpdateSieve {
 }  // namespace
 
 LoopbackServer::LoopbackServer(const base::FilePath& persistent_file)
-    : version_(0),
+    : strong_consistency_model_enabled_(false),
+      version_(0),
       store_birthday_(0),
       persistent_file_(persistent_file),
       observer_for_tests_(nullptr) {
@@ -271,6 +272,10 @@ void LoopbackServer::HandleCommand(
   SaveStateToFile(persistent_file_);
 }
 
+void LoopbackServer::EnableStrongConsistencyWithConflictDetectionModel() {
+  strong_consistency_model_enabled_ = true;
+}
+
 bool LoopbackServer::HandleGetUpdatesRequest(
     const sync_pb::GetUpdatesMessage& get_updates,
     sync_pb::GetUpdatesResponse* response) {
@@ -323,6 +328,20 @@ string LoopbackServer::CommitEntity(
     return string();
   }
 
+  // If strong consistency model is enabled (usually on a per-datatype level,
+  // but implemented here as a global state), the server detects version
+  // mismatches and responds with CONFLICT.
+  if (strong_consistency_model_enabled_) {
+    EntityMap::const_iterator iter = entities_.find(client_entity.id_string());
+    if (iter != entities_.end()) {
+      const LoopbackServerEntity* server_entity = iter->second.get();
+      if (server_entity->GetVersion() != client_entity.version()) {
+        entry_response->set_response_type(sync_pb::CommitResponse::CONFLICT);
+        return client_entity.id_string();
+      }
+    }
+  }
+
   std::unique_ptr<LoopbackServerEntity> entity;
   syncer::ModelType type = GetModelType(client_entity);
   if (client_entity.deleted()) {

components/sync/engine_impl/loopback_server/loopback_server.h

@@ -53,6 +53,9 @@ class LoopbackServer {
                      int64_t* response_code,
                      std::string* response);
 
+  // Enables strong consistency model (i.e. server detects conflicts).
+  void EnableStrongConsistencyWithConflictDetectionModel();
+
  private:
   // Allow the FakeServer decorator to inspect the internals of this class.
   friend class fake_server::FakeServer;
@@ -177,6 +180,8 @@ class LoopbackServer {
     observer_for_tests_ = observer;
   }
 
+  bool strong_consistency_model_enabled_;
+
   // This is the last version number assigned to an entity. The next entity will
   // have a version number of version_ + 1.
   int64_t version_;

components/sync/test/fake_server/fake_server.cc

@@ -441,6 +441,11 @@ void FakeServer::DisableNetwork() {
   network_enabled_ = false;
 }
 
+void FakeServer::EnableStrongConsistencyWithConflictDetectionModel() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  loopback_server_->EnableStrongConsistencyWithConflictDetectionModel();
+}
+
 base::WeakPtr<FakeServer> FakeServer::AsWeakPtr() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return weak_ptr_factory_.GetWeakPtr();

components/sync/test/fake_server/fake_server.h

@@ -164,6 +164,9 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   // This can be used to trigger exponential backoff in the client.
   void DisableNetwork();
 
+  // Enables strong consistency model (i.e. server detects conflicts).
+  void EnableStrongConsistencyWithConflictDetectionModel();
+
   // Implement LoopbackServer::ObserverForTests:
   void OnCommit(const std::string& committer_id,
                 syncer::ModelTypeSet committed_model_types) override;