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