697126 - Add X-Backoff to token server and other non-storage APIs

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Token, defect)

Description

[Mike Connor [:mconnor]]

can add the same const magic to the code, but should document this if nothing else and let it be a Zeus impl detail...

Comment 1

[Toby Elliott [:telliott]]

Not much point here, since user API is going away eventually.

Comment 2

[Mike Connor [:mconnor]]

Does the token server API support this header?

Comment 3

[Richard Newman [:rnewman]]

Morphing this bug.

Comment 4

[Richard Newman [:rnewman]]

rfkelly, please close as appropriate if new code does the right thing…

Comment 5

[Ryan Kelly [:rfkelly]]

I don't think this is documented as part of the tokenserver API, and there's nothing in the code that would generate it currently.  We should split these details off into a generic "Services HTTP APIs all support the following headers" section of the docs...

Comment 6

[Ryan Kelly [:rfkelly]]

Attachment: tokenserver-backoff.diff

Let's get the standard Retry-After/X-Backoff pair into the tokenserver docs in preparation for fxa+sync launch; we can refactor the docs to make this a standard thing later on.

Comment 7

[Ryan Kelly [:rfkelly]]

(Alternately we could just use "Backoff" since "X-*" headers are out of vogue these days)

Comment 8

[Richard Newman [:rnewman]]

Hah, I was just about to ping you about this! Occurred to me in the shower this morning. 

Filed Bug 958795 for Android.

Mark, Tim, Chris: not sure where you're currently at with tracking this kind of stuff for the desktop client, so headsup!

Comment 9

[Mike Connor [:mconnor]]

For a bit of context: Making this work was a P0 from SvcOps in the days of the Instant Sync meltdown. tl;dr it's really good to have an explicit way to tell clients to sync less frequently. 503+Retry-After presumes the service is down, 20x + X-Backoff is a valid way to successfully serve clients while still pushing them into less-frequent syncing.

sidenote: we should have unit tests to ensure that all client code understands Retry-After and X-Backoff are Very Important for scheduling.  Manual syncs MAY override, but automatic sync intervals MUST follow backoff headers.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Mike Connor [:mconnor] from comment #9)

> sidenote: we should have unit tests to ensure that all client code
> understands Retry-After and X-Backoff are Very Important for scheduling. 
> Manual syncs MAY override, but automatic sync intervals MUST follow backoff
> headers.

Sometimes I think that the Android Sync codebase is actually an elaborate backoff-handling system with accompanying unit tests, and Fennec is a favicon management app that also displays HTML.

Comment 11

[Chris Karlof [:ckarlof]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Hah, I was just about to ping you about this! Occurred to me in the shower
> this morning. 
> 
> Filed Bug 958795 for Android.
>
> Mark, Tim, Chris: not sure where you're currently at with tracking this kind
> of stuff for the desktop client, so headsup!

Bug 958447 already exists for desktop.

Comment 12

[Toby Elliott [:telliott]]

Comment on attachment 8358315 [details] [diff] [review]
tokenserver-backoff.diff

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

I think that when we actually do more extensive protocol work, these should be merged or given more distinct semantics, but that's a separate conversation from documenting them (which is fine here)

Comment 13

[Ryan Kelly [:rfkelly]]

Committed in https://github.com/mozilla-services/docs/commit/400ff966ac1539ee8b8874b1f543cdc52fbbd705

Do we want to keep this bug open to track more open-ended work on the protocol for this?

Comment 14

[Ryan Kelly [:rfkelly]]

unblocking TS metabug since this made it into the docs

Comment 15

[Chris Karlof [:ckarlof]]

Ryan, can we close this?

Comment 16

[Ryan Kelly [:rfkelly]]

Yes We Can

Comment 17

[James Bonacci [:jbonacci]]

Done.