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 { diff --git a/t/188-balancer_keepalive_pool_max_retry.t b/t/188-balancer_keepalive_pool_max_retry.t index 679ee680f8..6588312ab5 100644 --- a/t/188-balancer_keepalive_pool_max_retry.t +++ b/t/188-balancer_keepalive_pool_max_retry.t @@ -6,7 +6,7 @@ use Cwd qw(cwd); log_level('info'); repeat_each(1); -plan tests => repeat_each() * (blocks() * 6); +plan tests => repeat_each() * (blocks() * 6 - 1); my $pwd = cwd(); @@ -87,3 +87,130 @@ __DATA__ [200, 502] --- no_error_log eval 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; + + proxy_next_upstream_tries 3; + upstream backend { + server 0.0.0.1; + keepalive 4; + 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. + -- 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 + + 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 + } + } + + 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) + 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") + } + } + + location / { + return 444; + } + } +--- 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; + proxy_set_header Connection ""; + } + + location = /bad { + proxy_pass http://backend; + proxy_http_version 1.1; + proxy_set_header Connection ""; + proxy_next_upstream error timeout; + proxy_next_upstream_timeout 0; + } +--- request +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]