bugfix: balancer_by_lua* fix unbounded retries caused by a wrapped try count

[Teachh]

set_more_tries() can grant a wrapped try count, which makes proxy_next_upstream_tries unenforceable: a single downstream request retries until the client disconnects.

I hereby granted the copyright of the changes in this pull request to the authors of this lua-nginx-module project.

The defect

ngx_http_lua_ffi_balancer_set_more_tries() reduces the requested count with:

max_tries = r->upstream->conf->next_upstream_tries;
total = bp->total_tries + r->upstream->peer.tries - 1;

if (max_tries && total + count > max_tries) {
    count = max_tries - total;
    *err = "reduced tries due to limit";
}

bp->more_tries = count;

max_tries and total are ngx_uint_t. Once total exceeds max_tries the subtraction is negative; count is an int so it holds that negative value, and it is then stored into bp->more_tries, which is ngx_uint_t. peer.tries consequently receives a value near 2^64 and decrements from there, so the termination check can never pass.

Instrumented, with proxy_next_upstream_tries 3:

set_more_tries max_tries:3 total:1 granted:1
set_more_tries max_tries:3 total:3 granted:0
set_more_tries max_tries:3 total:4 granted:-1
set_more_tries max_tries:3 total:5 granted:-2
get_peer cached:0 tries:18446744073709551614
get_peer cached:0 tries:18446744073709551613

total passes max_tries because nginx re-increments peer.tries for errors on cached connections (ngx_http_upstream_next(), if (u->peer.cached && ft_type == NGX_HTTP_UPSTREAM_FT_ERROR)). A balancer that asks for one extra try on every invocation — which is what ingress-nginx's balancer does — therefore grows the budget faster than it depletes.

This contradicts the documented contract for set_more_tries():

Please note that, the total number of tries in a single downstream request cannot exceed the hard limit configured by proxy_next_upstream_tries.

Reproduction

A standalone configuration is also included at the end of this description, for reproducing it outside the test suite. It needs no Lua application code: nginx is its own backend and return 444 closes a connection without sending a response, which is what an upstream crashing mid-request looks like. Warm the upstream keepalive pool with concurrent requests, then send one request to the failing path.

Concurrency during the warm-up is required — a single cold request stays within the limit, because nothing has yet inflated peer.tries.

Measured on nginx 1.31.4 + this module at master, 3 trials per configuration, counting prematurely closed lines in the error log for a unique path:

balancer calls set_more_tries(1) per invocation	upstream keepalive	upstream attempts
yes	yes	200, 200, 4 (guard hit 2/3)
yes	no	200, 200, 200 (guard hit 3/3)
plain proxy_pass	yes	5, 4, 5
plain proxy_pass	no	5, 4, 5

Calling set_more_tries(1) only on the first invocation of each request stays bounded: 5, 7, 5.

Verification of the fix

Rebuilt with the clamp, same reproduction, 6 trials: 6, 5, 5 with keepalive and 5, 6, 5 without. Every request returned a 502 within milliseconds and the guard was never reached — 0 storms in 6 trials, against 5 of 6 before.

Test

A companion test is proposed for openresty/lua-resty-core as t/balancer.t TEST 23, since this is ngx.balancer behaviour: openresty/lua-resty-core#PENDING.

It is deterministic and needs no concurrency. The trick is distinct peer addresses: 127.0.0.1, 127.0.0.2 and 127.0.0.3 are separate keepalive pool entries, so three sequential warm-up requests leave one cached connection each. Retries then land on cached connections, which are not charged against peer.tries, letting bp->total_tries outgrow max_tries. With proxy_next_upstream_tries 2 and set_more_tries(1) the fourth request answers 502 with the fix and 500 without it, because the loop trips the test's own invocation guard.

Note that a grant larger than 1 is not sufficient on its own: the existing guard clamps count so that total + count <= max_tries whatever the grant, and while a negative count does appear it is harmless in that case — −1 becomes 2^64−1 in the unsigned more_tries, so peer.tries += more_tries merely decrements by one. The failure needs |count| > peer.tries, which takes several consecutive cached-connection failures.

Context

Found while investigating a production incident on ingress-nginx v1.12.8, where one HTTP request to a path with no matching route produced roughly 392,000 upstream retries and 6.5M log lines in seven minutes, saturating a shared Kafka logging topic. The client received no response at all rather than a 502.

Related history: #866 was superseded by #913, whose C change was merged as da11870db65e — the guard this patch corrects. Its companion test, proposed in openresty/lua-resty-core#59, does not appear in any commit in that repository, so this path has been untested since. #1546 was rejected citing the contract quoted above.

Reproduction configuration

# Reproduces unbounded upstream retries via set_more_tries().
#
#   mkdir -p logs && nginx -p $PWD -c REPRO-lua-storm.conf
#   # warm the upstream keepalive pool with concurrent requests:
#   seq 32 | xargs -P32 -I{} curl -s -o /dev/null http://127.0.0.1:8080/warm
#   # then one request to a path whose backend closes without responding:
#   curl -s -m 5 -o /dev/null -w '%{http_code}\n' http://127.0.0.1:8080/bad
#   grep -c 'prematurely closed' logs/error.log
#
# Expected with proxy_next_upstream_tries 3: at most 3 upstream attempts, then
# a 502. Actual: the balancer is re-entered until the client gives up.
#
# The "GUARD hit" branch exists only to stop the loop for this reproduction;
# without it the request never terminates on its own.

master_process on;
error_log logs/error.log notice;
pid logs/nginx.pid;
events { worker_connections 1024; }

http {
    # adjust to your own checkouts
    lua_package_path "/path/to/lua-resty-core/lib/?.lua;/path/to/lua-resty-lrucache/lib/?.lua;;";
    access_log off;

    # The backend is nginx itself. "return 444" closes the connection without
    # sending a response, which is what an upstream crashing mid-request does.
    server {
        listen 8081;
        location = /warm { return 200 "ok\n"; }
        location /       { return 444; }
    }

    upstream backend {
        server 0.0.0.1;   # placeholder, replaced by the balancer
        keepalive 32;     # not required to reproduce, but matches real setups

        balancer_by_lua_block {
            local b = require "ngx.balancer"

            local n = (ngx.ctx.n or 0) + 1
            ngx.ctx.n = n
            ngx.log(ngx.WARN, "BALANCER invocation ", n)
            if n > 200 then
                ngx.log(ngx.ERR, "GUARD hit")
                return ngx.exit(500)
            end

            -- One extra try on every invocation, retries included. This is
            -- the pattern ingress-nginx's balancer uses.
            local ok, err = b.set_more_tries(1)
            if err then
                ngx.log(ngx.WARN, "set_more_tries: ", err)
            end

            assert(b.set_current_peer("127.0.0.1", 8081))
        }
    }

    server {
        listen 8080;
        location / {
            proxy_pass http://backend;
            proxy_http_version 1.1;
            proxy_set_header Connection "";
            proxy_next_upstream error timeout;
            proxy_next_upstream_tries 3;
            proxy_next_upstream_timeout 0;
        }
    }
}