905887 - Sometimes, the API responds that a collection has been created before it exists

Status: VERIFIED FIXED

(Marketplace Graveyard :: API, defect, P1)

Description

[Matt Basta [:basta]]

If you POST a new collection, you'll get a 2XX response code. If you immediately GET the collection you just POSTed, there's a chance you'll get a 404.

The API should not close the connection and return the 2XX response code until the collection has decidedly been created.

Comment 1

[Matt Basta [:basta]]

I should note that waiting a few seconds and GETting a second time causes the same request to complete successfully.

Comment 2

[Mathieu Pillard [:mat]]

master-slave db related problems ? Do we handle that elsewhere, e.g. for apps ?

Comment 3

[Wil Clouser [:clouserw]]

We solved it in the past with master-pinning (setting a very short lived cookie which routes that user to do reads from the master.).  Happy to have a better system though...maybe when we save it we also dump it in memcache so the next get will succeed?

Comment 4

[Matt Basta [:basta]]

This isn't so much of an issue because we can fudge the internal caches with cache rewriting using the response of the POST, but if the user refreshes the page too soon, the request will fail.

Also, PATCHing the collection or adding an app before the collection has propagated to all the DB servers could be a concern.

Comment 7

[Mathieu Pillard [:mat]]

This also happens for translations, which is even more annoying, because sometimes the returned data is missing the translations entirely. See bug 907969 which was just duped for this one.

Comment 10

[Chuck Harmston [:chuck]]

For posterity: we do run 1 master and 4 slaves on dev, and this is likely replication lag. We're looking into solutions on both the hardware and software side.

Comment 11

[Matt Basta [:basta]]

I can do some tomfoolery on the client side to mask this (when you create something, we obviously have a copy of the data already and can just fill in missing translated fields in the cache), but the whole 404 on refresh is something we can't do much about. I'm also very hesitant to make a client-side patch for a server-side issue because I don't want the core problem to get swept under the rug.

Comment 12

[Mathieu Pillard [:mat]]

FWIW it looks like we already have db pinning mechanism in place in zamboni, for the Marketplace and not just AMO:

We use django-db-router's multidb.PinningMasterSlaveRouter. We have our own middleware, in mkt/api/middleware.py, APIPinningMiddleware, that replaces the standard PinningRouterMiddleware. According to its docstring:
- For the API, authenticated calls will always happen on the master
- For everything else, it falls back on the cookie thing (which is put on every response following a POST, and lasts 15 seconds or so).

Comment 13

[Andy McKay]

Is the GET after the POST anonymous or authenticated?

Comment 14

[Matt Basta [:basta]]

Authenticated.

Comment 16

[Christopher Van Wiemeersch [:cvan]]

Krupa says this is a blocker to the Curation Tool going live. Can someone look at this?

Comment 17

[Matt Basta [:basta]]

https://github.com/mozilla/rocketfuel/commit/65f9099c2dd51988463d4053daaa34dc0c56bfa2

This should fix the symptoms where the name/description don't show up, but doesn't solve the refresh issue.

Comment 18

[Mathieu Pillard [:mat]]

So, we decided to dupe the bugs but there are 2 separate, subtly different issues here:

- The original issue, which is still a problem : if you create a collection and request it immediately afterwards, there is a chance you'll get a 404 if your GET request is not authenticated, because it'll then use the slave.

- The translations part, which are read from the slave even when you're authenticated, and even when returning the object you just created, in the POST/PATCH request.

The culprit for the second issue, that basta worked around in comment 17:
https://github.com/diox/zamboni/blob/master/apps/translations/transformer.py#L70

All translations that are automatically fetched for all objects, no matter what the context is, are returned from the slave. This is going to be tricky to fix, I assume it's here for a reason. I wrote an overview of all the stuff that happens behind the scenes with Translations in zamboni to get started : https://gist.github.com/diox/e99d74a8e445032b49fa

Comment 19

[Mathieu Pillard [:mat]]

Worth noting that Victor and I just reproduced this on -dev with a simple App as well. Submit an app, if you are unlucky enough, the consumer page & reviewer tools queue will show the name & description as blank for a few minutes (my guess is, the slave delay is very short here, but the issue is amplified by django-cache-machine ; in the test I just made, it took 10 minutes for consumer pages & reviewer tools to finally show me the name)

Comment 20

[Christopher Van Wiemeersch [:cvan]]

Rob landed a fix for this yesterday:
https://github.com/mozilla/zamboni/commit/6f51de0

Basta has confirmed the issue is fixed. If anyone runs into further problems, holler!

If there's still more to do, Mathieu, please reopen and take the bug.

Thanks, Rob, Mat, everyone!

Comment 21

[Mathieu Pillard [:mat]]

We need to review+push https://github.com/mozilla/zamboni/pull/1144 otherwise translations will still be read from the slave, even when we are pinned on the master.

Comment 22

[Mathieu Pillard [:mat]]

Completely fixed (translations part) in https://github.com/mozilla/zamboni/commit/20eeccef34cc9a88325e86d4798f28c5c88446f2

STR:
- Create a collection in the curation tool on -dev
- Once it's done, check the collection in the curation tool, make sure your new collection name & description show up properly.

Since it also affects other code in a more subtle way:
- Submit an app on -dev
- Immediately afterwards, check its name in reviewer tools queue and consumer pages. It should not be blank.

Comment 23

[Victor Carciu]

Verified both scenarios and everything is working as expected now.