1051739 - Re-enable "409 Conflict" responses from sync storage server

Status: RESOLVED WONTFIX

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

Description

[Ryan Kelly [:rfkelly]]

Per Bug 959034 and Bug 959032, Firefox Sync clients currently do not have very good handling for the "409 Conflict" response code.  On desktop it's likely to produce a visible error bar.

Let's disable this response temporarily, until we get actual support for it from the clients.  This patch replaces it with a "503 Service Unavailable" with short retry-after, which will trigger approximately the desired behaviour of waiting briefly then trying again.  We can then leave this bug open, depending on the client bugs, to track re-enabling this response.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: syncstorage-no-409-conflict.diff

It's not nice but it will make for slightly nicer client behaviour...

Comment 2

[Ryan Kelly [:rfkelly]]

Committed here:
https://github.com/mozilla-services/server-syncstorage/commit/bd58e657db2a528406df655ffd6675756bfbe0e5

Comment 3

[Ryan Kelly [:rfkelly]]

Updating the bug title from "disable" to "re-enable" pending the linked client bugs.

Comment 4

[Ryan Kelly [:rfkelly]]

If I correctly understand the linked bugs, the discussion in Bug 1034377, and my own vague memory, the primary reason we disabled this because it caused too many error bars.

We no longer have error bars.  I wonder if it's safe to re-enable this in the interests of better logging and server diagnostics.  :markh :dcoates :rnewman, thoughts?

Comment 5

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

I don't see how this would cause problems other than the fact the "retry" probably isn't going to be as timely as it should be - 409 isn't going to check for backoffs, so I think it's going to wait for the next scheduled sync to try again (and possibly enter a client-initiated backoff if the errors persist - but we'd have to be extremely unlucky to hit this multiple times), when it probably should attempt the retry (almost) immediately.

(The scheduler is a mess - it's very difficult to rationalize what it does by reading it)

Comment 6

[Ryan Kelly [:rfkelly]]

> I think it's going to wait for the next scheduled sync to try again

Is that a 5-minute interval?  That doesn't seem too bad, although if I understand the current behaviour correctly, we currently send a 503 with five-second retry period.

Comment 7

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

(In reply to Ryan Kelly [:rfkelly] from comment #6)
> > I think it's going to wait for the next scheduled sync to try again
> 
> Is that a 5-minute interval?

10 minutes for a multi-device user (or an hour if they go idle.) I'm leaning towards thinking that if we do this, we should get a P1 client bug on file to do better (but I'm inclined to agree 10 mins on the off-chance this is hit isn't *too* bad)

Comment 8

[Danny Coates [:dcoates]]

It sounds like the experience with 409 enabled is still worse. The current rate of these[1] is < 1000 per day, a minuscule number. I'm in favor of doing nothing for now.


[1] - https://kibana.shared.us-west-2.prod.mozaws.net/#/dashboard/elasticsearch/PROD%20-%20Sync%20App%20Errors

Comment 9

[Richard Newman [:rnewman]]

I don't see much reason to disagree with Mark and Danny on this :D

What we _could_ do is return 409s for the new batch API only. Those clients should understand conflict responses, given that the entire purpose of that API is to achieve consistency.



(In reply to Ryan Kelly [:rfkelly] from comment #6)
> we currently send a 503 with five-second retry period.

Five seconds is probably too short for all collections apart from clients, FWIW. I'd probably go with 30 to avoid the possibility of partials.

Comment 10

[Ryan Kelly [:rfkelly]]

Alright, let's leave well enough alone then :-)