<div dir="ltr"><div>Thank you for your reply. It makes sense to me. I didn't intend to </div><div>change the behavior. I thought the implementation doesn't </div><div>match the docs and didn't notice the docs have clarified this </div><div>behavior.<br><br></div><div>Now I understand it better. For the cases of error, timeout, and </div><div>invalid_header, ngx_http_upstream_next is called directly. Since </div><div>they are always considered unsuccessful attempts, there is no </div><div>need to check the ft_type against u->conf->next_upstream. For other</div><div>cases, ngx_http_upstream_test_next has already checked the status </div><div>against  u->conf->next_upstream to determine whether to call </div><div>ngx_http_upstream_next, so there is also no need to check again.</div><div><br></div><div>I have another question though. When peer.free is called from </div><div>ngx_http_upstream_next, the state argument is correctly passed.<br></div><div>But for the last attempt that doesn't finally enter </div><div>ngx_http_upstream_next, peer.free is called from </div><div>ngx_http_upstream_finalize_request with the state being 0, in this case </div><div>the last attempt is always considered successful. But in fact it may </div><div>be unsuccessful. So I think the status should also be checked there </div><div>in order to pass the correct state. I have created a PR for this. Could </div><div>you have a look? Thanks.<br><a href="https://github.com/nginx/nginx/pull/959">https://github.com/nginx/nginx/pull/959</a></div></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>> 于2025年10月31日周五 00:46写道：<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello!<br>
<br>
On Thu, Oct 30, 2025 at 02:48:54PM +0800, solomon wrote:<br>
<br>
> # HG changeset patch<br>
> # User Zhefeng Chen <<a href="mailto:logchen2009@gmail.com" target="_blank">logchen2009@gmail.com</a>><br>
> # Date 1761731339 -28800<br>
> #      Wed Oct 29 17:48:59 2025 +0800<br>
> # Node ID b81b918263d445c32cb2cff5e2724e3cac62dab3<br>
> # Parent  c3be8460587196f376bccf29817c3a6523e18150<br>
> Upstream: max_fails doesn't respect the next_upstream config<br>
> <br>
> In the description of `max_fails`, it says:<br>
> What is considered an unsuccessful attempt is defined by the<br>
> `proxy_next_upstream`, `fastcgi_next_upstreami`,<br>
> `uwsgi_next_upstream`, `scgi_next_upstream`,<br>
> `memcached_next_upstream`, and `grpc_next_upstream` directives.<br>
> <br>
> But actually 403 and 404 are always considered an successful<br>
> attempt, while other cases are always considered an<br>
> unsuccessful attempt.<br>
> <br>
> The `ngx_http_upstream_free_round_robin_peer` function depends<br>
> on this to update the health check state.<br>
<br>
The documentation of the proxy_next_upstream directive (and other <br>
directives mentioned) further clarifies the behaviour <br>
(<a href="http://freenginx.org/r/proxy_next_upstream" rel="noreferrer" target="_blank">http://freenginx.org/r/proxy_next_upstream</a>):<br>
<br>
: The directive also defines what is considered an unsuccessful <br>
: attempt of communication with a server. The cases of error, <br>
: timeout and invalid_header are always considered unsuccessful <br>
: attempts, even if they are not specified in the directive. The <br>
: cases of http_500, http_502, http_503, http_504, and http_429 are <br>
: considered unsuccessful attempts only if they are specified in the <br>
: directive. The cases of http_403 and http_404 are never considered <br>
: unsuccessful attempts.<br>
<br>
Note that error, timeout, and invalid_header cases are always <br>
considered unsuccessful (because they are, indeed, unsuccessful, <br>
and there isn't much to be done here).  This is what you probably <br>
mean by "other cases are always considered an unsuccessful <br>
attempt".  This doesn't apply to all other cases though - <br>
http_500, http_502, http_503, http_504, and http_429 are only <br>
considered unsuccessful if they are explicitly specified in the <br>
directive.  These are valid responses, but at the same time these <br>
are error responses, so depending on the particular use case <br>
[free]nginx can either accept them as is (and send to the client) <br>
or try to fallback to a different server.  See <br>
ngx_http_upstream_test_next() for details on where these codes are <br>
checked.<br>
<br>
Note well that 403 and 404 are explicitly excluded from being <br>
considered unsuccessful attempts.  This behaviour is intentional <br>
and designed to support some common use cases, notably:<br>
<br>
1. When resources are distributed among multiple servers, and <br>
"proxy_next_upstream http_404;" is used to iterate over available <br>
servers till the requested resource is found.  Switching off a <br>
server if the resource is not found in such a setup is not desired <br>
for obvious reasons.<br>
<br>
2. Similarly, but with resources rsynced to all servers, so <br>
"proxy_next_upstream http_404 http_403;" makes it possible to <br>
fallback to other servers if the resource is not yet synced to a <br>
particular server (or the new directory is being synced, see <br>
5231:05c53652e7b4).<br>
<br>
Could you please elaborate a bit more on why do you suggest to <br>
change this behaviour?  And how the mentioned use cases are <br>
expected to be handled with the new behaviour?  Is it intentional <br>
that error, timeout, and invalid_header cases won't be considered <br>
unsuccessful unless explicitly specified in the <br>
proxy_next_upstream directive with the proposed change?  <br>
And invalid_header won't be considered unsuccessful by default?<br>
<br>
[...]<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
</blockquote></div>