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

RESOLVED FIXED

Status

Cloud Services
Firefox Sync: Backend
P1
normal
RESOLVED FIXED
7 years ago
11 months ago

People

(Reporter: philikon, Assigned: eoger)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Comment hidden (empty)

Comment 1

7 years ago
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.
(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.
(Reporter)

Updated

6 years ago
Depends on: 693893
(Reporter)

Updated

6 years ago
Blocks: 568569

Updated

6 years ago
Blocks: 744320
rfkelly, does Sync 2.0 support everything that we need to do this?
Flags: needinfo?(rfkelly)
Duplicate of this bug: 568569
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.
Flags: needinfo?(rfkelly)
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?
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".
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!
Duplicate of this bug: 605221
Duplicate of this bug: 1345700
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.
Priority: -- → P1

Updated

a year ago
See Also: → bug 1346438

Updated

11 months ago
Assignee: nobody → eoger
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

11 months ago
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
(Assignee)

Updated

11 months ago
Status: NEW → ASSIGNED

Comment 15

11 months ago
mozreview-review
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 ;)
Attachment #8847250 - Flags: review?(markh) → review+

Comment 16

11 months ago
mozreview-review
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.
Attachment #8847251 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

11 months ago
Thank you mark, landing.

Comment 20

11 months ago
Pushed by eoger@mozilla.com:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf5d88ce5097
https://hg.mozilla.org/mozilla-central/rev/8c9589795211
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.