Page MenuHomePhabricator

query service should send a retry-after header with a 429 response
Closed, ResolvedPublic

Description

WikibaseQualityConstraints respects 429 responses as of T204469
However the query service right now still does not send a retry-after header with the 429 response, it probably should.

Event Timeline

Restricted Application added a project: Wikidata. · View Herald TranscriptJan 17 2019, 12:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Addshore moved this task from incoming to monitoring on the Wikidata board.Jan 17 2019, 4:36 PM

I thought we do send Retry-After. Can we get a reproducing case? Maybe some proxy drops it? Because I am looking at the throttling code and clearly see producing the Retry-After header.

Yup, that code looks like it is definitely sent.

I guess we need to look at both the nginx proxy and the varnish proxy?
Traffic might know the answer for the varnish layer.

Can we get a reproducing case?

We could try hammering the endpoint?
There is a log line including response headers from the WDQS referenced in T204267#4887839. That is what it sees.

Well, as mentioned in T204267#4865669, we’re currently looking for Retry-After, case-sensitive. And if I remember correctly, nginx usually sends headers in all lowercase. So unless MediaWiki’s MWHttpRequest::getResponseHeaders normalizes retry-after back into Retry-After, we won’t find the header.

Change 485625 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Look for Retry-After header case-insensitively

https://gerrit.wikimedia.org/r/485625

Well, as mentioned in T204267#4865669, we’re currently looking for Retry-After, case-sensitive. And if I remember correctly, nginx usually sends headers in all lowercase. So unless MediaWiki’s MWHttpRequest::getResponseHeaders normalizes retry-after back into Retry-After, we won’t find the header.

That isn't the issue here, the client still does not get the header.

This can be seen in the logs on T204267#4887839 which is reported by https://github.com/wikimedia/mediawiki-extensions-WikibaseQualityConstraints/blob/1ef719f5891a616450e4fc21cfd49527a456b7d1/src/ConstraintCheck/Helper/LoggingHelper.php#L298 and dumps all of the headers into the log.

{"responseHeaders":"{\"server\":[\"nginx\\/1.13.6\"],\"date\":[\"Tue, 15 Jan 2019 15:27:10 GMT\"],\"content-type\":[\"application\\/octet-stream\"],\"connection\":[\"close\"],\"cache-control\":[\"no-cache\"]}"

Also getResponseHeaders and getResponseHeader do normalization of the header using the parseHeader method.
So we should actually only ever be looking for lowercase (I'll add this to code review)

Change 485625 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Fix Retry-After header handling

https://gerrit.wikimedia.org/r/485625

Smalyshev triaged this task as Normal priority.Feb 1 2019, 7:35 AM

I tried with https://query.wikidata.org/sparql?throttleMe=1&query=SELECT%20*%20WHERE%20%7B%20?x%20?y%20?z%20%7D%20LIMIT%2010 which always produces 429, and indeed I do not see Retry-After. Moreover, I also do not see X-Served-By header too. Looks like something is stripping headers here. Same happens on localhost, so it's probably nginx. I'll dig into nginx configs and try to figure out why.

Change 491870 had a related patch set uploaded (by Smalyshev; owner: Smalyshev):
[operations/puppet@production] Turn off proxy_intercept_errors for nginx

https://gerrit.wikimedia.org/r/491870

Change 491870 merged by Gehel:
[operations/puppet@production] Turn off proxy_intercept_errors for nginx

https://gerrit.wikimedia.org/r/491870

Gehel added a subscriber: Gehel.

Patch merged, let me know if the issue isn't fixed.

Addshore closed this task as Resolved.Feb 21 2019, 3:44 PM
Addshore claimed this task.

I just tested this with a call to https://query.wikidata.org/sparql?throttleMe=1&query=SELECT%20*%20WHERE%20%7B%20?x%20?y%20?z%20%7D%20LIMIT%2010 and I see a Retry-After :)
I would consider this resolved.

Restricted Application added a project: User-Addshore. · View Herald TranscriptFeb 21 2019, 3:44 PM