1205901 - Two-way Synchronization History Data Adapter

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Sync, defect, P2)

Description

[Sean Lee [:seanlee][:weilonge]]

The read-only history adapter is implemented for synchronizing History data from Firefox Sync Service in bug 1191773.

This bug will provide a two-way synchronization feature for pushing the records browsed in FxOS to FxSync Service.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: WIP - Two-way sync feature

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/DataSync/master/Bug1205901 > mozilla-b2g:master

Comment 3

[Sean Lee [:seanlee][:weilonge]]

After having a discussion with Michiel, we come out a new design based on the first prototype (attachment 8671998 [details] [review]).

Step 1: sync-engine fetches changes from remote (DataAdapter#handleConflict may be called), and DataAdapter#update is called
Step 2: Retrieve the changes in Places Data Store by using checkIfClearedSince [1] (rename to getChanges for this, and add a callback for handleTask), and if task.operation === 'add' or 'create' ->
         Kinto Collection.get
         2.1 No FxSync ID exists, and it means it's a new record -> Kinto Collection#create (without `id` field), write back fxsyncId into DS
         2.2 FxSync ID exists -> Kinto Collection#update (merge visits array, check title field)

Step 3:
      Write History Collection records to PlacesDS. That`s a read-only part and we`ve implemented. (Kinto Collection -> Local DS).
      Return true or false to DataAdapter#update

Step 4: (handled by sync-engine; DataAdapter#handleConflict may be called)
      Push all waiting-for-syncing records to FxSync and update last synced revision id.

[1] https://github.com/michielbdejong/gaia/blob/1224183-DataAdapter-refactor/shared/js/places_model.js#L351

Comment 4

[Michiel de Jong [:michielbdejong]]

Yes! :) Cool that we have a clear plan for this now.

Just found a typo in our etherpad text:
* s/task.operation === 'add' or 'create'/task.operation === 'add' or 'update'

And we forgot to write down that DataAdapter#handleConflict should do the same thing as when a DS add or update triggers a Kinto Collection#update, namely merge visits array, and pick newest title field.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,


I got the following error [1] at [2] when the history record is tried to create in KintoColleciton.
Could you help to give me a suggestion about this? Thank you.

attachment 8671998 [details] [review] has my latest patch.

[1] {
  "ok": false,
  "lastModified": 1451361144080,
  "errors": [
    {
      "path": "/v1/buckets/611c7e96a16778e40016504c1928fc57/collections/history/records/MYAiun7IIj8x",
      "sent": {
        "id": "MYAiun7IIj8x",
        "payload": "{\"hmac\":\"39577f7792e88a7a527cbdf4cda35c71486def154b7dcc2de380711fcdd1846b\",\"ciphertext\":\"0Z51Jq1RM7AkTi1k+mCL3HbuuE36rWURHLoCpYCy86O5V95vCJ7KDPqFe8+fEjI2MhZAKXzcW+0btfLduyDe0SpC3aMClTq4arsgVMoP1epdCkW61MWsTv6zjA77BvsnDi1ML9NtaPkj0nlFcSjQHafjgU1MaK19AofMCpK/HsAOuTpo3EWKWV8mTBl/NuOB7wYYOUBTFdXR0zB1qoNwCw==\",\"IV\":\"CRutp5QNWNgicxYHrz9mWw==\"}"
      },
      "error": {
        "errno": 999,
        "message": "Failed batch subrequest",
        "code": 400,
        "error": "Bad Request"
      },
      "type": "outgoing"
    }
  ],
  "created": [],
  "updated": [],
  "deleted": [],
  "published": [],
  "conflicts": [],
  "skipped": [],
  "resolved": []
}
[2] https://github.com/weilonge/gaia/blob/648a120df01795c26b885bc4611ae3d86b1a84f2/shared/js/sync/engine.js#L282

Comment 6

[Michiel de Jong [:michielbdejong]]

Which Syncto server are you using? Make sure it's configured to allow read-write traffic (the dev/prod Syncto instances don't).

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Attachment: HTTP 412 error screenshot

Hi Natim, Michiel,

When a new record is tried to sync-up to remote service, the record can be uploaded successfully. The process doing to Collection is [create -> update -> sync], and it's verified by using Kinto.js library directly or SyncEngine.

When the existing and modified records are tried to sync-up, Sync Engine will exit accidentally with this error [1]: (see the attachment)
HTTP 412; Resource was modified meanwhile: Resource was modified meanwhile

It's shown the same result when using Collection to [update -> sync].

Do you have any idea for this issue? Thank you.


[1] {
    "errno": 114,
    "message": "Resource was modified meanwhile",
    "code": 412,
    "error": "Precondition Failed"
}

Comment 8

[Michiel de Jong [:michielbdejong]]

Sounds like a client-side issue, will try to replicate.

Comment 9

[Rémy Hubscher (:natim)]

You might want to use the CLIENT_WINS strategy: http://kintojs.readthedocs.org/en/latest/api/#synchronization-strategies

Comment 10

[Sean Lee [:seanlee][:weilonge]]

After giving strategy: Kinto.syncStrategy.CLIENT_WINS, it still shows the same error:
Error: HTTP 412; Resource was modified meanwhile: Resource was modified meanwhile

Comment 11

[Michiel de Jong [:michielbdejong]]

Found the issue, it was server-side after all, created  https://github.com/mozilla-services/syncto/issues/78

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Besides, MANUAL is what SyncEngine wants. SyncEngine will handle the conflict with this:
 https://github.com/mozilla-b2g/gaia/blob/master/shared/js/sync/engine.js#L263-L268

Comment 13

[Michiel de Jong [:michielbdejong]]

Issue now fixed in Syncto, try updating to latest master.

Comment 14

[Rémy Hubscher (:natim)]

Ok so I just released Cliquet 2.15.0 with the bugfix.

I am ready to do the Syncto 1.5.0 release: https://github.com/mozilla-services/syncto/pull/80

Please tell me if the latest master works for you guys and I will do the release.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Hi Natim,

Thanks a lot for your update!

After applying prepare-1.5.0 branch of Syncto, 412 error is gone! There is a conflict object in SyncResult when an existing record tried to be sync-up, but the conflict object contains a 'null' remote object.
So I use local-win strategy manually. Here is another error message from Kinto.js: 

Error Message:  "e.remote is null"

Stack:
"[208]</Collection</<.value@app://sync.gaiamobile.org/shared/js/sync/ext/kinto.min.js:632:10604SyncEngine.prototype._resolveConflicts/SyncEngine</<@app://sync.gaiamobile.org/shared/js/sync/engine.js:266:15"

Could you help to give some suggestions? Thank you.

Comment 16

[Michiel de Jong [:michielbdejong]]

That probably comes from https://github.com/mozilla-b2g/gaia/pull/32263/files#diff-4d73ff90f2479c9b5d1f53457e85e25dR591 can you show the exact conflict object that is logged by the two console.log lines directly above that? Let's chat about it on irc!

Comment 17

[Sean Lee [:seanlee][:weilonge]]

It's the conflict object:

{
  "type": "outgoing",
  "local": {
    "id": "ifNjxr2SZZ2r",
    "payload": "{\"hmac\":\"116348c8f601b3fcde88cf0cd43309581b63a88aad409df7f51aa917161be740\",\"ciphertext\":\"u1eoar3cy310k8OvRH1MovftYTaUKYFACAiUce/I3tQdHf1pWMnRKVZz4oAh9jsK52bGzjROM6+iGTqjBrL0Ywgqllbrxsn+bliBLyLIZJ+orHVfL3GBZhBvCvlYqljjoPDe/swMh7czavOkS3cLOjWTRZHa4wfNP+Xs9YLNgSeZ4dC91dhRF6tawITvi06sAp+IdIQ9aD4WBeoTZ5ofd3LqyoN5wvQD7bu9WnYPbBU=\",\"IV\":\"36xgz+KRxN6g3DpFFkkopQ==\"}"
  },
  "remote": null
}

Comment 18

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Sean, were you able to intercept the server response for that fetch operation? I'm really surprised with the remote property being null, as we mostly forward what we received from the server here.

Comment 19

[Rémy Hubscher (:natim)]

It is quite possible that the Firefox Sync server is returning a 412 without returning the previous record.

I am pretty sure you should not pass the ``If-None-Match`` header for this records.
Kinto.js seems to think that you are creating a new record when you are actually trying to update it.
It looks like the last_modified value is not kept when you are updating the record.

Could this be a problem with the way the encryption/decryption transformer works?

Comment 20

[Michiel de Jong [:michielbdejong]]

Ah, makes sense, if there's a conflict during PUT, there will be no response body to the 412 response. So would need to do an extra GET after a 412 (I think we already do that anyway?).

But because pullChanges runs before pushChanges, it surprises me that the data on the server changes inbetween the pull and the push. It seems much more likely that pullChanges triggers an incoming conflict than that pushChanges triggers an outgoing conflict, right?

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Attachment: sync_traffic.json

Hey Niko,

All sync traffic is included in this attachment.
In the last entry, it's a POST request at line 2720 .

And then SyncResult contains this conflict object:
{
  "type": "outgoing",
  "local": {
    "id": "ifNjxr2SZZ2r",
    "payload": "{\"hmac\":\"3ba109d63b84900a8c6d34533cda7a0f05f5a2514b2f38320f9274718f5410e1\",\"ciphertext\":\"ffJy21yQTYeKCfQnsus9/C29Au/wEF2BY4OwzNok0Dn91OjrwY2QyH6fnmD4ACpNatyjTqtgxaRvGjchmUR6aakuqmtpwZG1qhmDZnQ97vNSeTbdEkq0P+STahpL3kx5H3ympEQ4WIOIIzieLpO2rYTtke9hZ5xJEE2ZCBGkJtXu0UBtZ2DbUKEbcoLIWs07yh0SkXHpdd/+psTsYgCvh0Go/meubd5AQD6f3n5c5ww9bmTQNSHmfy6tKTTAA/ARx9FajgFcXe4pcLrRTq0//Q==\",\"IV\":\"FKjTmUM62rVWNInRHUDtjg==\"}"
  },
  "remote": null
}

Comment 22

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

So it's confirmed it's the server returning a 412 batch response with errno 114 "Resource was modified meanwhile" with no conflicting record attached to its json body, that's why we get a null client side.

We should now investigate why is that, could be a server issue or a malformed client request. I think :michielbdejong has a clue it's the latter and is currently working on a patch, am I right Michiel?

Comment 23

[Michiel de Jong [:michielbdejong]]

Yes, try with the code from https://github.com/Kinto/kinto.js/pull/309 I think that should stop the 412s from happening.

Comment 24

[Sean Lee [:seanlee][:weilonge]]

Attachment: sync_log_with_kintojs_fix.har

Hey Michiel,

After applying the latest kinto.js, it still shows me the following error:
{
  lineNumber: 8480, // [1]
  message: "conflict.remote is null",
  stack: "resolve@app://sync.gaiamobile.org/shared/js/sync/ext/kinto.min.js:8480:9SyncEngine.prototype._resolveConflicts/SyncEngine</<@app://sync.gaiamobile.org/shared/js/sync/engine.js:266:15"
}

You can see the last POST request at line 2208 in this attachment.

kinto.js is built from 'npm run build' command with this commit of Kinto.js : 
commit 701019790d01722769e78ee06b3117b293565f3c
Merge: 3cc9b38 a5c8553
Author: Mathieu Leplatre <[e-mail]>
Date:   Mon Feb 1 23:42:14 2016 +0100

    Merge pull request #181 from Kinto/181-subresource-integrity

    Add Sub-Resource Integrity information on dist/ files

[1] https://github.com/weilonge/gaia/blob/ab6add8b024d7413cffb629eefc9f8a43a503a46/shared/js/sync/ext/kinto.min.js#L8480

Comment 25

[Sean Lee [:seanlee][:weilonge]]

The error seems at here:
https://github.com/Kinto/kinto.js/blob/master/src/collection.js#L768

Comment 26

[Michiel de Jong [:michielbdejong]]

Looks like you're now running into https://github.com/Kinto/kinto.js/issues/306 (although, did you `make clean` to remove your profile from earlier tests?)

Comment 27

[Sean Lee [:seanlee][:weilonge]]

(In reply to Michiel de Jong [:michielbdejong] from comment #26)
> Looks like you're now running into
> https://github.com/Kinto/kinto.js/issues/306 (although, did you `make clean`
> to remove your profile from earlier tests?)

I always run 'make clean && make && launch b2g'