On a first thought, this seems an unsolvable problem with the current server-side eventual consistency model: if two clients commit the same entity concurrently, the last one will win, and the same outcome (whatever that is) will be visible in both clients.

So let's say client A and client B are not syncing. On day 1 client A deletes the address. On day 2 client B updates the address. Then on day 3 they both enable sync back (or fix an auth error for example). Can we expect a deterministic outcome then?

So basically, when you say commit concurrently, is it relative to when the data changed or to when sync saw the change?

We must distinguish two scenarios here:
A. The clients had sync disabled (which makes it unlikely that they had the same address in the first place, but perhaps they had been syncing before and later disabled sync).

B. Sync was enabled, but effectively not running (e.g. auth error or clients were offline).

For case A: the merge logic kicks in, and that should be perfectly deterministic: the update wins (there's no such a thing as a tombstone if sync is disabled).

For case B: I believe there's a race condition, and we need to distinguish two subscenarios:
B.1. Both clients come online at the very same time: due to the server's eventual consistency model, the conflict wouldn't be detected and the last one wins.

B.2. One client comes online earlier: that client should commit, and the other client (eventually) receive the update, detect the conflict, and resolve (conflict resolution being deterministic: update wins over deletion).


Tests seem to exercise scenario B.1 (IMO unsolvable). It's however suspicious that the directory-based implementation doesn't trigger this issue, so I'm not 100% sure.

Wrt B.2: we may have a bug there (orthogonal to USS), because I'm not confident that the invalidation (or the update) will be received *before* the commit in all cases.

To sum up, I don't see USS having any relevant (expected) differences here and I cannot explain why the directory-based implementation isn't affected.

One possible next step could be to fork this test (TwoClientAutofillProfileSyncTest.DeleteAndUpdate) in a setup where  the server operates in a strong consistency model (e.g. make LoopbackServer::CommitEntity() verify the base version, i.e. SyncEntity.version in the protocol).

sync-triage ping

I''ll prepare a patch.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7767cc3513f5c459329268807478909b77f3e898

commit 7767cc3513f5c459329268807478909b77f3e898
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Oct 01 20:30:29 2018

Add autofill sync test for deterministic conflict resolution

TwoClientAutofillProfileSyncTest.DeleteAndUpdate can only expect a
deterministic outcome if the server operates with a strong consistency
model.

In order to prove that, we introduce a second test that enables such
mode and is correspondingly more strict about the final outcome (update
should override deletion).

Bug:  870333 
Change-Id: Ie4e6f5ebd604c837103237168391ba05595d1b22
Reviewed-on: https://chromium-review.googlesource.com/1249206
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595542}
[modify] https://crrev.com/7767cc3513f5c459329268807478909b77f3e898/chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc
[modify] https://crrev.com/7767cc3513f5c459329268807478909b77f3e898/components/sync/engine_impl/conflict_resolver.cc