692700 - Use 'X-If-Unmodified-Since' header when uploading meta/global and crypto/keys to avoid race condition

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Comment 1

[Gregory Szorc [:gps]]

Does the server even support this?

I don't think it applies in this case (since each client would theoretically generate a unique value), but in case it does, it is generally preferred to use If-None-Match instead of If-Unmodified-Since for conditional uploads. With If-Unmodified-Since, you still have race conditions within a 1s window +- clock skew. With If-None-Match, you have an entity tag that changes with the underlying bits, so you are guaranteed not to upload the same entity, even if the timestamps are packed close together. Of course, you still have a race condition on 2 web nodes hitting the underlying store (unless you lock on the store or do an atomic operation), but I digress. See RFC 2616 13.3.3 (http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.3.3) for more on these weak and strong validators.

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Gregory Szorc [:gps] from comment #1)
> Does the server even support this?

I wouldn't be filing this if it didn't ;). See http://docs.services.mozilla.com/storage/apis-1.1.html#headers

> I don't think it applies in this case (since each client would theoretically
> generate a unique value), but in case it does, it is generally preferred to
> use If-None-Match instead of If-Unmodified-Since for conditional uploads.

I agree. This is a good thought to keep in mind for a Sync Storage API 1.2 or 2.0 revision.

Comment 3

[Richard Newman [:rnewman]]

rfkelly, does Sync 2.0 support everything that we need to do this?

Comment 5

[Ryan Kelly [:rfkelly]]

X-If-Unmodified-Since-Version should allow you to avoid race conditions when uploading meta/global or crypto/keys individually.  If you need to treat both of them as an atomic unit then you're out of luck, since X-I-U-S-V can only be applied within a single collection.

Note that X-If-Unmodified-Since-Version will avoid the timestamp-resolution concerns highlighted in Comment 1, and the sync2.0 spec guarantees no race conditions between webheads working on the same store.

Comment 6

[Richard Newman [:rnewman]]

Do we get a version in response to GET info/collections, so that we can use XIUSV? Or do we need to GET meta/global and receive a 404 with a version?

Comment 7

[Ryan Kelly [:rfkelly]]

Yes, GET info/collections will give you the last-modified version of each collection, so you can pick out the one for "meta" and use it directly in X-I-U-S-V.

If you receive a 404 from GET meta/global it won't give you an explicit version number, because the thing you're asking about doesn't exist.  You should use X-I-U-S-V=0 to condition requests on "the target resource does not exist".

Comment 8

[Richard Newman [:rnewman]]

OK, so for a blank server, require v=0 for all of the "fresh start" objects. For partial recovery, we can use the times from the last info/collections fetch; if that causes a failure, it's due to concurrent modification.

Sounds good, thanks Ryan!

Comment 11

[Mark Hammond [:markh] [:mhammond]]

We've discovered this could cause bad things to happen in some race conditions. It's not yet clear if this is related to the spike in HMAC errors we've seen recently, but either way, we should do this as a priority.

Comment 12

[Edouard Oger [:eoger]]

Attachment: Bug 692700 part 1 - Set X-If-Unmodified-Since when uploading meta/global.

Review commit: https://reviewboard.mozilla.org/r/120254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120254/

Comment 13

[Edouard Oger [:eoger]]

Attachment: Bug 692700 part 2 - Set X-If-Unmodified-Since when uploading crypto/keys.

Review commit: https://reviewboard.mozilla.org/r/120256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120256/

Comment 14

[Edouard Oger [:eoger]]

I think we should open a follow-up to that bug and clean-up/refactor the disabled/enabled engines management.
Currently it is done in two different places, [0] and [1].
I also noticed that when we disable an engine, we actually send 2 PUT meta/global requests (1 from each place).


[0]: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/stages/enginesync.js#244
[1]: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js#1097-1114

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8847250 [details]
Bug 692700 part 1 - Set X-If-Unmodified-Since when uploading meta/global.

https://reviewboard.mozilla.org/r/120254/#review124328

::: services/sync/modules/service.js:1132
(Diff revision 1)
> -    // Unfortunately, the servers don't support it after a wipe right now
> -    // (bug 693893), so we're going to defer this until bug 692700.
> +   * @param {WBORecord} meta meta/global record
> +   * @param {Number} lastModified known last modified timestamp (in decimal seconds),
> +   *                 will be used to set the X-If-Unmodified-Since header
> +   * @throws the response object if the request was not a success
> +   */
> +  _uploadMetaGlobal(meta, lastModified) {

Given everyone other than `_uploadNewMetaGlobal()` calls this with `(meta, meta.modified)`, I wonder if `_uploadNewMetaGlobal` could just set its .modified to zero, and we avoid the 2nd param here?

::: services/sync/modules/service.js:1133
(Diff revision 1)
> -    // (bug 693893), so we're going to defer this until bug 692700.
> +   * @param {Number} lastModified known last modified timestamp (in decimal seconds),
> +   *                 will be used to set the X-If-Unmodified-Since header
> +   * @throws the response object if the request was not a success
> +   */
> +  _uploadMetaGlobal(meta, lastModified) {
> +    this._log.debug("Uploading meta/global: " + JSON.stringify(meta));

`this._log.debug("Uploading meta/global", meta);`

::: services/sync/modules/service.js:1140
(Diff revision 1)
> +    res.setHeader("X-If-Unmodified-Since", lastModified);
>      let response = res.put(meta);
>      if (!response.success) {
>        throw response;
>      }
> +    // "Successful responses will return the new last-modified time for the collection."

not clear why this comment is in quotes (or not clear what it is quoting ;)

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8847251 [details]
Bug 692700 part 2 - Set X-If-Unmodified-Since when uploading crypto/keys.

https://reviewboard.mozilla.org/r/120256/#review124336

Looks great, thanks.

::: services/sync/modules/service.js
(Diff revision 1)
>      this._log.info("Encrypting new key bundle.");
>      wbo.encrypt(this.identity.syncKeyBundle);
>  
> -    this._log.info("Uploading...");
> -    let uploadRes = wbo.upload(this.resource(this.cryptoKeysURL));
> +    let uploadRes = this._uploadCryptoKeys(wbo, 0);
> +    if (!uploadRes.success) {
> -    if (uploadRes.status != 200) {

IIRC, any 2XX response will set .success, and while I agree it shouldn't matter I don't think we should risk this change in this patch - it might bite us later.

::: services/sync/modules/service.js:1151
(Diff revision 1)
> +   * @param {WBORecord} cryptoKeys crypto/keys record
> +   * @param {Number} lastModified known last modified timestamp (in decimal seconds),
> +   *                 will be used to set the X-If-Unmodified-Since header
> +   */
> +  _uploadCryptoKeys(cryptoKeys, lastModified) {
> +    this._log.debug("Uploading crypto/keys");

might as well add lastModified to the log message.

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8847250 [details]
Bug 692700 part 1 - Set X-If-Unmodified-Since when uploading meta/global.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120254/diff/1-2/

Comment 18

[Edouard Oger [:eoger]]

Comment on attachment 8847251 [details]
Bug 692700 part 2 - Set X-If-Unmodified-Since when uploading crypto/keys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120256/diff/1-2/

Comment 19

[Edouard Oger [:eoger]]

Thank you mark, landing.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf5d88ce5097
part 1 - Set X-If-Unmodified-Since when uploading meta/global. r=markh
https://hg.mozilla.org/integration/autoland/rev/8c9589795211
part 2 - Set X-If-Unmodified-Since when uploading crypto/keys. r=markh

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf5d88ce5097
https://hg.mozilla.org/mozilla-central/rev/8c9589795211