_handleUnexpectedResponse should probably throw for non 200 errors - or it's got to do something else special, else we end up with: > 1428570983146 readinglist.sync ERROR Unexpected response downloading modified items: {"status":401,... > 1428570983149 readinglist.sync INFO Sync done > 1428570983150 readinglist.scheduler INFO Sync completed successfully 1428570983155 readinglist.scheduler INFO next scheduled sync is in [2 hours] Bug 1148980 will cause it to be an error log (assuming it lands) but it still leaves us with the "2 hours" thing - we need to enter an "error" schedule. Maybe we could be conservative and have _handleUnexpectedResponse set a flag and throw at the very end - or maybe there's a flag on the engine the scheduler could check? Either way, I think this is important - it seems like something that might cause a perception of "it never syncs"

This patch throws on 401 or 5XX, which I think is a reasonable first start. However, there is a risk we'll strike a server bug causing a 50X on just a single item which we could ignore and still get a "mostly successful" sync, but I'm not sure that's a good reason to try and force our way forward in these error states.

P2 based on "I think this is important - it seems like something that might cause a perception of "it never syncs", but it also sorta sounds like this is just a logging fix?

(In reply to Justin Dolske [:Dolske] from comment #2) > P2 based on "I think this is important - it seems like something that might > cause a perception of "it never syncs", but it also sorta sounds like this > is just a logging fix? Actually, it also involves the scheduler - without this fix we wait 2 hours after an error before we retry. With this fix we exponentially back off to do 2mins, 4mins, 8mins retry until either success or 2 hours.

On reviewing your "todo" patch, I thought that maybe the first version of this patch was too aggressive - that aborting on those response codes in "sub" responses could cause issues. I think we only need to abort when the "top-level" response is 401 or 5XX (eg, I could imagine a server bug might cause one single item to always return 5XX even though the response itself worked as did every other item in the body. So this version adds a bool to _handleUnexpectedResponse(), which indicates if the response is top-level. We then only throw when it is true. Drew, do you think this is an improvement on the earlier one?

Comment on attachment 8592572 [details] [diff] [review] 0002-Bug-1152698-certain-readinglist-server-response-code.patch Approval Request Comment [Feature/regressing bug #]: readinglist [User impact if declined]: The readinglist sync engine will only retry every 2 hours on error instead of by the normal error schedule which is more frequent. [Describe test coverage new/current, TreeHerder]: Existing tests pass [Risks and why]: Risk limited to Readinglist syncing. [String/UUID change made/needed]: None

Mark, do we still need to uplift this to 38 and 39 if we're aiming for a future release or would it make more sense to keep this on 40 for now?

Comment on attachment 8592572 [details] [diff] [review] 0002-Bug-1152698-certain-readinglist-server-response-code.patch No need to uplift this