Closed Bug 958447 Opened 6 years ago Closed 6 years ago

Client support for tokenserver backoff protocol

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: rfkelly, Assigned: markh)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file, 1 obsolete file)

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 → ---
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
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+
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+
(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.
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+
https://hg.mozilla.org/mozilla-central/rev/dd003bff09ee
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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?
Attachment #8390233 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 969228
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.