Client support for tokenserver backoff protocol

RESOLVED FIXED in Firefox 29

Status

Cloud Services
Firefox Sync: Backend
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rfkelly, Assigned: markh)

Tracking

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29+ fixed, firefox30 fixed)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Per Bug 697126 the tokenserver may issue 200+X-Backoff and/or 503+Retry-After responses, clients should be prepared to handle them as gracefully as possible.
Whiteboard: [qa+]
Depends on: 697126
Assignee: nobody → jparsons
Target Milestone: --- → mozilla29
Sorry.  Ignore me.  Bug 945449 is for the auth server; this is for the token server.
Status: NEW → ASSIGNED
Target Milestone: mozilla29 → ---
(Assignee)

Updated

4 years ago
Blocks: 976836
Status: ASSIGNED → NEW
Priority: -- → P1
Assignee: jparsons → nobody
Assignee: nobody → ckarlof
N.B., worthwhile just accepting X-Backoff anywhere we accept X-Weave-Backoff.
Mark has a patch in progress for this.
Assignee: ckarlof → mhammond
(Assignee)

Comment 5

4 years ago
Created attachment 8389982 [details] [diff] [review]
0001-Bug-958447-respect-Retry-After-header-from-token-ser.patch

This patch looks for a Retry-After header on *any* error response from the token server.  If it exists, it informs sync of the retry interval so the next sync should not be scheduled until that time.

Note however that the *current* sync may still continue to request tokens until that sync completes (presumably with an error) - or to put it another way, this header does not cause sync to abort the current sync - and completing that sync may still request further tokens - up to one token request per engine.  Does that sound acceptable?  If not, the patch would become much more complex.

Also note there is no support for X-Backoff as that doesn't seem to make much sense for the token server - on a 200 (with or without that header), we now have a token, so don't need to request a new one until the it expires.  Upon expiry we just request a new one regardless of any X-Backoff specified - and that request will either succeed, or fail with a Retry-After.  It doesn't seem to make sense to preemptively fail here (and what might make more sense would be for the token server to simply increase the token validity such that another request isn't going to be made in that period anyway).

Casting a wide feedback net:

rnewman to verify that sending the weave:service:backoff:interval notification during the sync will do the right thing (code inspection tells me it should)

ckarlof to verify the tokenserverclient changes are sane.

rfkelly to verify the limitations described above will still keep services happy.
Attachment #8389982 - Flags: feedback?(rnewman)
Attachment #8389982 - Flags: feedback?(rfkelly)
Attachment #8389982 - Flags: feedback?(ckarlof)
Comment on attachment 8389982 [details] [diff] [review]
0001-Bug-958447-respect-Retry-After-header-from-token-ser.patch

Review of attachment 8389982 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/common/tokenserverclient.js
@@ +377,5 @@
>        } else if (response.status == 404) {
>          error.message = "Unknown service.";
>          error.cause = "unknown-service";
>        }
> +      // A Retry-After header should theoretically only appear on a 503, but

Nit: newline before.

@@ +380,5 @@
>        }
> +      // A Retry-After header should theoretically only appear on a 503, but
> +      // we'll look for it on any error response.
> +      let retryAfter = response.headers['retry-after'];
> +      if (retryAfter) {

Also X-Backoff, and check for X-Backoff on success responses.
Attachment #8389982 - Flags: feedback?(rnewman) → feedback+
(Reporter)

Comment 7

4 years ago
Comment on attachment 8389982 [details] [diff] [review]
0001-Bug-958447-respect-Retry-After-header-from-token-ser.patch

I'm fine with ignoring the X-Backoff for expedience in the short-term, since having proper retry-after handing landed will be much more valuable.  But I'd like to see a follow-up bug for handling X-Backoff as well.

Even if it's not a full elegant backoff state-machine, ISTM there should be room to squeeze "let the current sync succeed but don't schedule another one for X seconds" somewhere into the flow.  Having the client know not to come back for a while is still better than having them show up and get a 503.
Attachment #8389982 - Flags: feedback?(rfkelly) → feedback+
(Assignee)

Comment 8

4 years ago
(In reply to Ryan Kelly [:rfkelly] from comment #7)
> Even if it's not a full elegant backoff state-machine, ISTM there should be
> room to squeeze "let the current sync succeed but don't schedule another one
> for X seconds" somewhere into the flow.  Having the client know not to come
> back for a while is still better than having them show up and get a 503.

Right - but the "strange" thing about that case is that the tokenserver gave us a valid token with an expiry.  So the end result here is that we got a token, but we will be telling sync to not try and use it for X seconds, even though it remains valid for some (or possibly even all) of those X seconds.

But after discussion with rnewman, that's exactly what we will do - even if we got a token, we will inform sync of the back-off, so the next Sync will not be scheduled until that backoff is met, even though it is possible we could have used that token to successfully sync during that period.

It still seems to me that returning a token with an extended expiry in this case might be a better option - this would mean we can sync as normal but avoid hitting the token server for a new token for the backoff period - but let's consider that later.
(Assignee)

Comment 9

4 years ago
Created attachment 8390233 [details] [diff] [review]
0001-Bug-958447-respect-Retry-After-header-from-token-ser.patch

Slightly different approach as discussed:
* tokenserverclient sends the notifications directly.
* policy.js looks for this tokenserver-specific notification but treats it identically to the existing sync-specific backoff notification.
* test checks that policy.js actually saw the notification.
Attachment #8389982 - Attachment is obsolete: true
Attachment #8389982 - Flags: feedback?(ckarlof)
Attachment #8390233 - Flags: review?(rnewman)
Comment on attachment 8390233 [details] [diff] [review]
0001-Bug-958447-respect-Retry-After-header-from-token-ser.patch

Review of attachment 8390233 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/tests/unit/test_browserid_identity.js
@@ +387,5 @@
> +  yield initializeIdentityWithTokenServerFailure({
> +    status: 503,
> +    headers: {"content-type": "application/json",
> +              "x-backoff": "200"},
> +    body: JSON.stringify({}),

"{}"? :)
Attachment #8390233 - Flags: review?(rnewman) → review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/dd003bff09ee
Status: NEW → ASSIGNED
status-firefox29: --- → affected
status-firefox30: --- → affected
tracking-firefox29: --- → ?
tracking-firefox29: ? → +
https://hg.mozilla.org/mozilla-central/rev/dd003bff09ee
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Comment 13

4 years ago
Comment on attachment 8390233 [details] [diff] [review]
0001-Bug-958447-respect-Retry-After-header-from-token-ser.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fxa Sync
User impact if declined: Sync Servers may become overloaded
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low 
String or IDL/UUID changes made by this patch: None
Attachment #8390233 - Flags: approval-mozilla-aurora?
status-firefox30: affected → fixed
Attachment #8390233 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 969228
You need to log in before you can comment on or make changes to this bug.