From e93ded3b54f327ceb7358bf54f76a743bdddc7ee Mon Sep 17 00:00:00 2001 From: Teachh Date: Mon, 14 Sep 2026 14:35:21 +0200 Subject: [PATCH 1/3] balancer_by_lua*: fix unbounded retries caused by a wrapped try count. set_more_tries() reduces the requested count with "count = max_tries - total", where both operands are ngx_uint_t. Once total exceeds max_tries the result is negative, and since count is an int it is then stored into bp->more_tries, which is ngx_uint_t. peer.tries therefore receives a value near 2^64 and decrements from there, so proxy_next_upstream_tries can never terminate the request: it retries until the client disconnects. 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), so a balancer that asks for one extra try on every invocation, as ingress-nginx's does, grows the budget faster than it depletes. Reproduction, with no Lua application code required: an upstream whose backend closes the connection without sending a response, warmed with 32 concurrent requests, then one further request. Before this change that request retried until the client timed out; after it, 5 to 7 attempts and a 502 within milliseconds, over 6 trials. A self-contained nginx.conf is attached to the pull request. No test is included: reproducing this needs concurrent warm-up before a single request, and Test::Nginx has no clean primitive for that. The attached configuration reproduces it deterministically instead. Observed in production on ingress-nginx v1.12.8, where one request produced roughly 392,000 upstream retries and 6.5M log lines in seven minutes. Signed-off-by: Teachh Co-Authored-By: Claude Opus 5 (1M context) --- src/ngx_http_lua_balancer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ngx_http_lua_balancer.c b/src/ngx_http_lua_balancer.c index aa26a4a07a..904de3a7d4 100644 --- a/src/ngx_http_lua_balancer.c +++ b/src/ngx_http_lua_balancer.c @@ -1192,7 +1192,9 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, total = bp->total_tries + r->upstream->peer.tries - 1; if (max_tries && total + count > max_tries) { - count = max_tries - total; + /* both operands are unsigned: total may already exceed max_tries, + * and the subtraction would then wrap instead of reducing count */ + count = total < max_tries ? (int) (max_tries - total) : 0; *err = "reduced tries due to limit"; } else { From c6af29c300c267995bce9d15a6e8f933516129fc Mon Sep 17 00:00:00 2001 From: lijunlong Date: Mon, 14 Sep 2026 23:06:42 +0800 Subject: [PATCH 2/3] add tests. --- t/188-balancer_keepalive_pool_max_retry.t | 121 +++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/t/188-balancer_keepalive_pool_max_retry.t b/t/188-balancer_keepalive_pool_max_retry.t index 679ee680f8..953342da4f 100644 --- a/t/188-balancer_keepalive_pool_max_retry.t +++ b/t/188-balancer_keepalive_pool_max_retry.t @@ -3,14 +3,16 @@ use Test::Nginx::Socket::Lua; use Cwd qw(cwd); +worker_connections(128); log_level('info'); repeat_each(1); -plan tests => repeat_each() * (blocks() * 6); +plan tests => repeat_each() * (blocks() * 6 - 1); my $pwd = cwd(); no_long_string(); +no_shuffle(); check_accum_error_log(); run_tests(); @@ -87,3 +89,120 @@ __DATA__ [200, 502] --- no_error_log eval qr/tries 7/ + + + +=== TEST 2: set_more_tries does not wrap after cached connection errors +--- http_config + lua_package_path "../lua-resty-core/lib/?.lua;;"; + lua_shared_dict warm_counter 1m; + + proxy_next_upstream_tries 3; + upstream backend { + server 0.0.0.1; + keepalive 32; + balancer_by_lua_block { + local b = require "ngx.balancer" + local n = (ngx.ctx.n or 0) + 1 + ngx.ctx.n = n + ngx.log(ngx.INFO, "balancer ", ngx.var.uri, " invocation ", n) + + -- This is the pattern used by ingress-nginx: grant one extra + -- attempt every time the balancer is entered, including retries. + if n > 20 then + ngx.log(ngx.ERR, "retry storm guard") + return ngx.exit(500) + end + + 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", $TEST_NGINX_RAND_PORT_1)) + } + } + + server { + listen 127.0.0.1:$TEST_NGINX_RAND_PORT_1; + + location = /warm { + content_by_lua_block { + local counter = ngx.shared.warm_counter + local n = counter:incr("requests", 1, 0) + while n < 32 do + ngx.sleep(0.01) + n = counter:get("requests") + end + ngx.say("ok") + } + } + + location / { + return 444; + } + } +--- config + location = /warm { + proxy_pass http://backend; + proxy_http_version 1.1; + proxy_set_header Connection ""; + } + + location = /bad { + proxy_pass http://backend; + proxy_http_version 1.1; + proxy_set_header Connection ""; + proxy_next_upstream error timeout; + } +--- init +use IO::Socket::INET; +use POSIX qw(_exit); + +my @pids; +for (1 .. 32) { + my $pid = fork(); + die "fork failed: $!" unless defined $pid; + + if ($pid == 0) { + my $sock; + for (1 .. 20) { + $sock = IO::Socket::INET->new( + PeerAddr => "127.0.0.1", + PeerPort => $Test::Nginx::Util::ServerPort, + Proto => "tcp", + Timeout => 1, + ); + last if $sock; + select undef, undef, undef, 0.1; + } + + _exit(1) unless $sock; + $sock->autoflush(1); + print $sock "GET /warm HTTP/1.1\r\n" + . "Host: localhost\r\n" + . "Connection: keep-alive\r\n\r\n"; + sysread($sock, my $buf, 4096); + close $sock; + _exit(0); + } + + push @pids, $pid; +} + +for my $pid (@pids) { + waitpid($pid, 0); + die "warm-up request failed" if $? != 0; +} +--- request +GET /bad +--- response_body_like: 502 Bad Gateway +--- error_code: 502 +--- grep_error_log eval: qr/balancer \/bad invocation [123]/ +--- grep_error_log_out +balancer /bad invocation 1 +balancer /bad invocation 2 +balancer /bad invocation 3 +--- no_error_log +retry storm guard +[alert] From b27160177b1855d5d568888a289009136edbc6a7 Mon Sep 17 00:00:00 2001 From: lijunlong Date: Mon, 14 Sep 2026 23:29:44 +0800 Subject: [PATCH 3/3] tests: correct the balancer retry underflow regression test. Warm four keepalive connections with concurrent subrequests and validate all warm-up responses. Bound the warm-up wait and match the complete balancer invocation sequence to detect wrapped retry budgets. Allow six attempts without the Nginx cached-error notification patch, and three with it. The previous guard rejected the bounded retry count before it could distinguish the fix from the original underflow. Verified that the old code fails and the fix passes without the Nginx notification patch. Repeated the fixed regression three times and passed all 11 assertions with the notification patch on both Lua variants. --- t/188-balancer_keepalive_pool_max_retry.t | 116 ++++++++++++---------- 1 file changed, 62 insertions(+), 54 deletions(-) diff --git a/t/188-balancer_keepalive_pool_max_retry.t b/t/188-balancer_keepalive_pool_max_retry.t index 953342da4f..6588312ab5 100644 --- a/t/188-balancer_keepalive_pool_max_retry.t +++ b/t/188-balancer_keepalive_pool_max_retry.t @@ -3,7 +3,6 @@ use Test::Nginx::Socket::Lua; use Cwd qw(cwd); -worker_connections(128); log_level('info'); repeat_each(1); @@ -12,7 +11,6 @@ plan tests => repeat_each() * (blocks() * 6 - 1); my $pwd = cwd(); no_long_string(); -no_shuffle(); check_accum_error_log(); run_tests(); @@ -93,6 +91,10 @@ qr/tries 7/ === TEST 2: set_more_tries does not wrap after cached connection errors +Without the Nginx cached-connection-error notification patch, four cached +connections are enough to drive total beyond proxy_next_upstream_tries. +The fix allows six attempts (four cached and two fresh); with the notification +patch the request stops after three. Neither case may wrap the retry budget. --- http_config lua_package_path "../lua-resty-core/lib/?.lua;;"; lua_shared_dict warm_counter 1m; @@ -100,7 +102,7 @@ qr/tries 7/ proxy_next_upstream_tries 3; upstream backend { server 0.0.0.1; - keepalive 32; + keepalive 4; balancer_by_lua_block { local b = require "ngx.balancer" local n = (ngx.ctx.n or 0) + 1 @@ -109,17 +111,27 @@ qr/tries 7/ -- This is the pattern used by ingress-nginx: grant one extra -- attempt every time the balancer is entered, including retries. - if n > 20 then + -- Allow the six bounded attempts even without the Nginx patch, + -- but stop a wrapped retry budget before the test times out. + if n > 6 then ngx.log(ngx.ERR, "retry storm guard") return ngx.exit(500) end local ok, err = b.set_more_tries(1) + if not ok then + error("failed to set more tries: " .. err) + end + if err then ngx.log(ngx.WARN, "set_more_tries: ", err) end - assert(b.set_current_peer("127.0.0.1", $TEST_NGINX_RAND_PORT_1)) + local ok, err = b.set_current_peer("127.0.0.1", + $TEST_NGINX_RAND_PORT_1) + if not ok then + error("failed to set current peer: " .. err) + end } } @@ -129,11 +141,20 @@ qr/tries 7/ location = /warm { content_by_lua_block { local counter = ngx.shared.warm_counter - local n = counter:incr("requests", 1, 0) - while n < 32 do + local n = counter:incr("requests", 1) + local deadline = ngx.now() + 2 + + -- Hold each response until all four upstream connections + -- are open, so the warm-up cannot reuse a single connection. + while n < 4 do + if ngx.now() >= deadline then + return ngx.exit(504) + end + ngx.sleep(0.01) n = counter:get("requests") end + ngx.say("ok") } } @@ -143,6 +164,30 @@ qr/tries 7/ } } --- config + location = /t { + content_by_lua_block { + ngx.shared.warm_counter:set("requests", 0) + + local responses = { ngx.location.capture_multi({ + { "/warm" }, + { "/warm" }, + { "/warm" }, + { "/warm" }, + }) } + + for i = 1, 4 do + local res = responses[i] + if res.status ~= 200 or res.body ~= "ok\n" then + error("warm-up failed: " .. res.status .. ": " .. res.body) + end + end + + ngx.say("warm-up: 4") + local res = ngx.location.capture("/bad") + ngx.say("status: ", res.status) + } + } + location = /warm { proxy_pass http://backend; proxy_http_version 1.1; @@ -154,55 +199,18 @@ qr/tries 7/ proxy_http_version 1.1; proxy_set_header Connection ""; proxy_next_upstream error timeout; + proxy_next_upstream_timeout 0; } ---- init -use IO::Socket::INET; -use POSIX qw(_exit); - -my @pids; -for (1 .. 32) { - my $pid = fork(); - die "fork failed: $!" unless defined $pid; - - if ($pid == 0) { - my $sock; - for (1 .. 20) { - $sock = IO::Socket::INET->new( - PeerAddr => "127.0.0.1", - PeerPort => $Test::Nginx::Util::ServerPort, - Proto => "tcp", - Timeout => 1, - ); - last if $sock; - select undef, undef, undef, 0.1; - } - - _exit(1) unless $sock; - $sock->autoflush(1); - print $sock "GET /warm HTTP/1.1\r\n" - . "Host: localhost\r\n" - . "Connection: keep-alive\r\n\r\n"; - sysread($sock, my $buf, 4096); - close $sock; - _exit(0); - } - - push @pids, $pid; -} - -for my $pid (@pids) { - waitpid($pid, 0); - die "warm-up request failed" if $? != 0; -} --- request -GET /bad ---- response_body_like: 502 Bad Gateway ---- error_code: 502 ---- grep_error_log eval: qr/balancer \/bad invocation [123]/ ---- grep_error_log_out -balancer /bad invocation 1 -balancer /bad invocation 2 -balancer /bad invocation 3 +GET /t +--- response_body +warm-up: 4 +status: 502 +--- grep_error_log eval: qr/balancer \/bad invocation \d+\b/ +--- grep_error_log_out eval +my $first = CORE::join "", map { "balancer /bad invocation $_\n" } 1 .. 3; +my $extra = CORE::join "", map { "balancer /bad invocation $_\n" } 4 .. 6; +qr/\A\Q$first\E(?:\Q$extra\E)?\z/ --- no_error_log retry storm guard [alert]