Skip to content

fix: do not prohibit HTTP/2 upstream on transient 429/503 - #2458

Open
2outside wants to merge 1 commit into
pymumu:masterfrom
2outside:fix/http2-429-no-prohibit
Open

fix: do not prohibit HTTP/2 upstream on transient 429/503#2458
2outside wants to merge 1 commit into
pymumu:masterfrom
2outside:fix/http2-429-no-prohibit

Conversation

@2outside

Copy link
Copy Markdown

Addresses the first item discussed in #2457.

Problem

_dns_client_http2_process_stream_one() sets server_info->prohibit = 1 for any
non-200 HTTP status:

int status = http2_stream_get_status(http2_stream);
if (status > 0 && status != 200) {
    tlog(TLOG_WARN, "http2 server query from %s:%d failed, server return http code: %d", ...);
    server_info->prohibit = 1;
    return -1;
}

For 429 Too Many Requests and 503 Service Unavailable that is disproportionate.
Those codes are backpressure aimed at the individual request; they do not say the
upstream is unavailable.

Following prohibit into _dns_client_check_server_prohibit()
(src/dns_client/dns_client.c), the flag causes _dns_client_shutdown_socket() to run,
so the shared HTTP/2 connection is torn down, and the server is skipped until
prohibit_time elapses. It is also self-reinforcing: the connection is re-established
and the next burst of queries runs into the same server-side limit again.

Some public DoH providers apply a per-connection request budget and answer 429 for the
excess. Because SmartDNS multiplexes all queries for one upstream over a single
connection, a normal burst — a batch of domains resolved at once, each producing A and
AAAA — can exceed it even when the average query rate is very low. On my routers one
upstream received roughly half the queries the other group members did, purely because it
kept cycling in and out of the prohibit state.

Change

Fail only the affected query for 429/503 and leave the connection and the server in
rotation. The caller already retries the query, and other members of the group answer in
the meantime. Every other status code keeps the previous behaviour.

Testing

Local nginx serving /dns-query over HTTP/2 with a self-signed certificate, proxying to
a real upstream so responses are valid, and configured to produce the failure mode:

limit_req_zone $binary_remote_addr zone=doh:1m rate=10r/s;
limit_req_status 429;
...
location /dns-query {
    limit_req zone=doh burst=10 nodelay;
    proxy_pass https://223.5.5.5/dns-query;
    ...
}

SmartDNS was configured with that endpoint as its only upstream, then hit with a burst of
30 concurrent queries. Recovery was measured as the time until the next query returned
NOERROR.

build not alive, prohibit logged time until next successful query
master 1 4s
this change 0 0s

Repeated three times with the same outcome for the prohibit count. The measured
recovery delay depends on which retry pass the query lands in (prohibit_time is 60 on
the first pass and 5 on the second), so the wait can be considerably longer than the 4s
observed here; with several upstreams in a group the practical effect is that a healthy
provider stops being used for that whole window.

Build is clean with no new warnings.

Not included

The concurrency/rate limiting discussed in #2457 is deliberately left out. I prototyped a
token-bucket pacer for the send path and measured it: pacing delays queries past
DNS_QUERY_TIMEOUT (500ms), which makes them time out and retry, so the total number of
requests — and of 429 responses — went up rather than down. That approach does not work
for a protocol whose own timeout is shorter than the burst it would need to absorb, so it
is not proposed here.

_dns_client_http2_process_stream_one() sets server_info->prohibit on any
non-200 HTTP status. For 429 (Too Many Requests) and 503 (Service
Unavailable) this is disproportionate: those codes are backpressure for the
individual request, not a statement that the upstream is unavailable.

Prohibiting the server calls _dns_client_shutdown_socket() and removes it
from rotation for prohibit_time seconds, so one 429 tears down the shared
HTTP/2 connection and excludes an otherwise healthy upstream. It is also
self-reinforcing: the connection is re-established and the next burst of
queries runs into the same server-side limit again.

Fail only the affected query, which the caller already retries, and leave the
connection and the server in rotation. Other status codes keep the previous
behaviour.

Tested against a local nginx DoH endpoint configured with limit_req and
limit_req_status 429. After a burst of 30 concurrent queries the unpatched
build logs "not alive, prohibit" and needs several seconds before the next
query succeeds; the patched build never prohibits the server and the next
query succeeds immediately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant