958447 - Client support for tokenserver backoff protocol

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Ryan Kelly [:rfkelly]]

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.

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Is this a dupe of Bug 945449?

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Sorry.  Ignore me.  Bug 945449 is for the auth server; this is for the token server.

Comment 3

[Richard Newman [:rnewman]]

N.B., worthwhile just accepting X-Backoff anywhere we accept X-Weave-Backoff.

Comment 4

[Chris Karlof [:ckarlof]]

Mark has a patch in progress for this.

Comment 5

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

Attachment: 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.

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[Ryan Kelly [:rfkelly]]

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.

Comment 8

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

(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.

Comment 9

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

Attachment: 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.

Comment 10

[Richard Newman [: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({}),

"{}"? :)

Comment 11

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

https://hg.mozilla.org/integration/fx-team/rev/dd003bff09ee

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd003bff09ee

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6373395a950a