From c4b6f393d5f8c781d2f96f47d9b3fb0f25c4c273 Mon Sep 17 00:00:00 2001 From: Andre Nogueira Date: Fri, 28 Aug 2026 12:01:42 +0100 Subject: [PATCH] fix(healthcheck): cold-start readiness gate for lazily-created, unprobed checkers (PS-12691) A freshly created health-check target defaults to internal_health=healthy with zero probes (add_target's hardcoded is_healthy=true), so a pod that restarts while its backend is already unhealthy briefly routes real traffic to it until enough active probes correct the target's state. Checker creation itself is also entirely lazy, seeded only by fetch_checker() on the live request path -- an idle-but-critical upstream with no prior traffic would never get a checker built ahead of a readiness check at all. There is currently no way to tell "healthy" (a real check passed) apart from "healthy" (the zero-probe default) from outside the checker, so nothing can gate readiness on "has this actually been checked yet." Adds two accessors to apisix/healthcheck_manager.lua for a readiness plugin to use: - ensure_checker(resource_path): proactively seeds a checker for a resource even with zero prior traffic, reusing the existing timer_create_checker construction path. Also resolves domain-name upstream nodes via parse_domain_in_up up front -- that resolution otherwise only happens on the live request path, so a checker built ahead of traffic would start probing under an unresolved identity and get silently rebuilt (wiping its probe count) the moment real traffic first resolves the domain. - is_resource_probed(resource_path): true only once every target of the resource's checker has had enough real active-check attempts for its state to have actually converged -- a single attempt is not always enough: with e.g. unhealthy.http_failures = 2 configured, internal_health only converges after two consecutive attempts. Computes the required attempt threshold from the checker's own config (max(unhealthy.http_failures, .tcp_failures, .timeouts, healthy.successes)) and delegates to a companion module function, resty.healthcheck.all_targets_probed(name, shm_name, min_attempts), proposed as a separate change against lua-resty-healthcheck-api7 (the vendored library this repo depends on): a per-target probe-attempt counter in shm, incremented each time an active check is actually dispatched for a target (success, failure, or timeout all count -- attempted, not "healthy"), queryable from any worker. t/node/healthcheck-fresh-node-default-healthy.t adds coverage for: lazy checker creation (fetch_checker returns false until the next timer tick), ensure_checker building a checker with zero prior traffic, all_targets_probed flipping only after a real probe, and the multi-attempt threshold behavior specifically (stays false after 1 attempt when min_attempts=2, flips true only after the 2nd). Validated end to end in a local kind cluster: pod-restart-while-unhealthy shows zero leaked requests before or after the readiness transition. Signed-off-by: Andre Nogueira --- apisix/healthcheck_manager.lua | 131 +++++++ .../healthcheck-fresh-node-default-healthy.t | 355 ++++++++++++++++++ 2 files changed, 486 insertions(+) create mode 100644 t/node/healthcheck-fresh-node-default-healthy.t diff --git a/apisix/healthcheck_manager.lua b/apisix/healthcheck_manager.lua index 1a69cf53245c..ac77c200f056 100644 --- a/apisix/healthcheck_manager.lua +++ b/apisix/healthcheck_manager.lua @@ -273,6 +273,137 @@ function _M.fetch_node_status(checker, ip, port, hostname) end +-- Cold-start readiness gate support (PS-12691): checker creation is +-- otherwise entirely lazy -- waiting_pool is only ever seeded by +-- fetch_checker(), which is only called from the live request path. An +-- upstream that receives zero traffic before a readiness probe fires would +-- never get a checker built at all, so a "has this been probed" gate would +-- hang forever for an idle-but-critical upstream on a fresh pod. This lets a +-- caller (the readiness plugin) force that seeding proactively. +-- +-- Scoped to plain, non-plugin-constructed resource paths (e.g. +-- "/upstreams/", "/routes/") -- unlike timer_create_checker, this +-- does not resolve the get_plugin_name()/construct_upstream branch used by +-- plugins like ai-proxy-multi. Critical upstreams for readiness gating are +-- configured as standalone apisixUpstreams entries, not inline plugin +-- config, so this scope is sufficient. +function _M.ensure_checker(resource_path) + if working_pool[resource_path] then + -- already created, by traffic or a previous ensure_checker call + return true + end + + local res_conf = resource.fetch_latest_conf(resource_path) + if not res_conf then + core.log.error("ensure_checker: resource not found: ", resource_path) + return false, "resource not found" + end + + local upstream = res_conf.value.upstream or res_conf.value + if not upstream.checks then + core.log.warn("ensure_checker: resource has no checks configured: ", resource_path) + return false, "no checks configured" + end + + -- Domain-name nodes are otherwise only resolved by apisix.upstream's + -- get_by_id -> parse_domain_in_up, which runs exclusively on the live + -- request path (apisix/init.lua). Without this, a checker seeded here + -- would start probing under the unresolved domain-string identity, then + -- get rebuilt -- wiping its accumulated shm state, including the probe + -- count is_resource_probed relies on -- the moment real traffic first + -- resolves the domain and bumps _nodes_ver. res_conf is the same shared + -- config object get_by_id operates on (both come from + -- core.config.fetch_created_obj), so has_domain/dns_nodes are already + -- populated by the config watcher's filter callback regardless of + -- traffic; resolve here so the checker is built once, under its final + -- identity, from the start. + if res_conf.has_domain then + local resolved, err = upstream_utils.parse_domain_in_up(res_conf) + if not resolved then + core.log.error("ensure_checker: failed to resolve domain nodes for ", + resource_path, ": ", err) + -- fall through with the unresolved config -- a subsequent real + -- request or ensure_checker call will retry the resolution + else + res_conf = resolved + upstream = res_conf.value.upstream or res_conf.value + end + end + + if not upstream.nodes or #upstream.nodes == 0 then + return false, "no nodes" + end + + local new_version = upstream_utils.version(res_conf.modifiedIndex, upstream._nodes_ver) + if waiting_pool[resource_path] ~= new_version then + core.log.info("ensure_checker: seeding waiting pool for ", resource_path, + " with version: ", new_version) + waiting_pool[resource_path] = new_version + end + return true +end + + +-- A single active-check attempt does not guarantee a target's real state is +-- known: with e.g. unhealthy.http_failures = 2 configured, internal_health +-- only converges after two consecutive failed attempts. Returns the +-- worst-case number of consecutive attempts needed to guarantee the real +-- state has been reached in either direction (healthy or unhealthy), or nil +-- if the resource has no active check configured at all -- in which case +-- there is nothing for a readiness gate to wait on. +local function required_probe_attempts(checks) + local active = checks and checks.active + if not active then + return nil + end + local unhealthy = active.unhealthy or {} + local healthy = active.healthy or {} + local max_attempts = 1 + for _, threshold in ipairs({ + unhealthy.http_failures, + unhealthy.tcp_failures, + unhealthy.timeouts, + healthy.successes, + }) do + if threshold and threshold > max_attempts then + max_attempts = threshold + end + end + return max_attempts +end + + +-- Cold-start readiness gate support (PS-12691): true only once every target +-- of the resource's checker has had enough real active-check attempts for +-- its state to have actually converged (see resty.healthcheck's +-- all_targets_probed -- "probed" means attempted, not "healthy", and one +-- attempt is not always enough, see required_probe_attempts above). false +-- if there is no live checker yet (ensure_checker not called, or +-- timer_create_checker hasn't run its next tick yet). +function _M.is_resource_probed(resource_path) + local item = working_pool[resource_path] + if not item or not item.checker or item.checker.dead then + return false + end + + local min_attempts = required_probe_attempts(item.checks) + if min_attempts == nil then + -- no active check configured: nothing for this gate to wait on. + return true + end + + if not healthcheck then + healthcheck = require("resty.healthcheck") + end + local ok, err = healthcheck.all_targets_probed(item.checker.name, healthcheck_shdict_name, min_attempts) + if ok == nil then + core.log.error("is_resource_probed: ", err) + return false + end + return ok +end + + local function add_working_pool(resource_path, resource_ver, checker, checks) working_pool[resource_path] = { version = resource_ver, diff --git a/t/node/healthcheck-fresh-node-default-healthy.t b/t/node/healthcheck-fresh-node-default-healthy.t new file mode 100644 index 000000000000..cd10626f3a04 --- /dev/null +++ b/t/node/healthcheck-fresh-node-default-healthy.t @@ -0,0 +1,355 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +use t::APISIX 'no_plan'; + +repeat_each(1); +log_level('info'); +no_root_location(); +no_shuffle(); + +run_tests(); + +__DATA__ + +=== TEST 1: a brand-new upstream's checker is not created until the async timer runs +# Reproduces PS-12691 / the IMP-3253 incident mechanism: apisix.healthcheck_manager +# .fetch_checker() (apisix/healthcheck_manager.lua) enqueues a never-before-seen +# resource into waiting_pool and returns nil for the request that triggered it -- +# the actual checker is only built later by the timer_every(1, ...) background +# timer (timer_create_checker). Neither #13627 nor #13629 (both in 3.18.0) touch +# this path: this test must still pass (i.e. still reproduce) on current HEAD. +# +# Wrap fetch_checker() itself (same monkey-patch style as +# healthcheck-incremental-update.t) so each call logs whether it got a live +# checker back, independent of internal resource_path/version bookkeeping. +--- extra_init_worker_by_lua + local healthcheck_manager = require("apisix.healthcheck_manager") + local orig_fetch_checker = healthcheck_manager.fetch_checker + local call_count = 0 + healthcheck_manager.fetch_checker = function(...) + call_count = call_count + 1 + local checker = orig_fetch_checker(...) + ngx.log(ngx.WARN, "fetch_checker call #", call_count, + " returned a live checker: ", tostring(checker ~= nil)) + return checker + end +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local checks = [[{ + "active":{ + "http_path":"/hello", + "timeout":1, + "type":"http", + "healthy":{ "interval":1, "successes":1 }, + "unhealthy":{ "interval":1, "http_failures":1 } + } + }]] + assert(t('/apisix/admin/routes/1', ngx.HTTP_PUT, [[{ + "upstream": { + "nodes": {"127.0.0.1:1980": 1}, + "type": "roundrobin", + "checks": ]] .. checks .. [[ + }, + "uri": "/hello" + }]]) < 300) + + -- first request: fetch_checker() has never seen this resource_path, + -- so it enqueues it into waiting_pool and returns nil this call -- + -- health filtering is bypassed for this request regardless of node health + t('/hello', ngx.HTTP_GET) + + ngx.sleep(1.5) -- let timer_create_checker build the checker + + -- second request: the checker now exists in the working pool + t('/hello', ngx.HTTP_GET) + + ngx.say("done") + } + } +--- request +GET /t +--- response_body +done +--- grep_error_log eval +qr/fetch_checker call #\d returned a live checker: (?:true|false)/ +--- grep_error_log_out +fetch_checker call #1 returned a live checker: false +fetch_checker call #2 returned a live checker: true +--- timeout: 5 + + + +=== TEST 2: a freshly added target defaults to healthy with zero probes +# create_checker()/sync_checker_targets() in apisix/healthcheck_manager.lua call +# checker:add_target(host, port, check_host, true, host_hdr) -- is_healthy is a +# hardcoded literal `true`. The vendored resty.healthcheck add_target() defaults +# a target to "healthy" the instant it is registered, before any probe has run. +# With healthy/unhealthy probe intervals set to 0 (disabled), no probe can ever +# fire, so any "healthy" reading here is provably the zero-probe default, not the +# result of a successful check. +--- config + location /t { + content_by_lua_block { + local healthcheck = require("resty.healthcheck") + + local checker = healthcheck.new({ + name = "test-ps12691-default-healthy", + shm_name = "upstream-healthcheck", + checks = { + active = { + healthy = { interval = 0 }, + unhealthy = { interval = 0 }, + }, + }, + events_module = "resty.events", + }) + if not checker then + ngx.say("failed to create checker") + return + end + + -- mirror healthcheck_manager.create_checker()'s call exactly: + -- add_target(host, port, check_host, true, host_hdr) + local ok, err = checker:add_target("127.0.0.1", 19791, nil, true, nil) + if not ok then + ngx.say("failed to add target: ", err) + return + end + ngx.sleep(0.2) -- let add_target's own event settle locally; no probe possible (interval=0) + + local status = checker:get_target_status("127.0.0.1", 19791) + ngx.say("fresh, never-probed target status: ", tostring(status)) + + checker:stop() + } + } +--- request +GET /t +--- response_body +fresh, never-probed target status: true +--- no_error_log +[error] +--- timeout: 5 + + + +=== TEST 3: all_targets_probed() is false until the first real active check fires +# Closes the gap TEST 2 exposes: a cold-start readiness gate needs a way to +# tell "healthy by default, never checked" apart from "healthy, actually +# checked" from outside the checker. resty.healthcheck.all_targets_probed() +# (hack/patches/lua-resty-healthcheck-probed-gate.patch) reads a per-target +# shm flag written the first time run_single_check actually dispatches a +# probe -- attempted, not "passed". Uses a real active interval (1s) against +# 127.0.0.1:1980, the standard t::APISIX mock backend, which answers /hello. +--- config + location /t { + content_by_lua_block { + local healthcheck = require("resty.healthcheck") + + local checker = healthcheck.new({ + name = "test-ps12691-all-targets-probed", + shm_name = "upstream-healthcheck", + checks = { + active = { + type = "http", + http_path = "/hello", + timeout = 1, + healthy = { interval = 1, successes = 1 }, + unhealthy = { interval = 1, http_failures = 1 }, + }, + }, + events_module = "resty.events", + }) + if not checker then + ngx.say("failed to create checker") + return + end + + local ok, err = checker:add_target("127.0.0.1", 1980, nil, true, nil) + if not ok then + ngx.say("failed to add target: ", err) + return + end + ngx.sleep(0.2) -- let add_target's own event settle locally + + local before = healthcheck.all_targets_probed( + "test-ps12691-all-targets-probed", "upstream-healthcheck") + ngx.say("before any active check has run: ", tostring(before)) + + ngx.sleep(1.5) -- past the 1s active.healthy.interval: one probe must have fired + + local after = healthcheck.all_targets_probed( + "test-ps12691-all-targets-probed", "upstream-healthcheck") + ngx.say("after the first active check: ", tostring(after)) + + checker:stop() + } + } +--- request +GET /t +--- response_body +before any active check has run: false +after the first active check: true +--- no_error_log +[error] +--- timeout: 5 + + + +=== TEST 4: ensure_checker() builds a checker with zero prior traffic +# Closes TEST 1's gap directly: healthcheck_manager.ensure_checker() +# (apisix/healthcheck_manager.lua) seeds waiting_pool proactively, without +# needing any request to fetch_checker() first -- unlike TEST 1, no t('/hello', +# ...) request is made at all before the checker is expected to exist. +--- extra_init_worker_by_lua + local healthcheck_manager = require("apisix.healthcheck_manager") + local orig_fetch_checker = healthcheck_manager.fetch_checker + local call_count = 0 + healthcheck_manager.fetch_checker = function(...) + call_count = call_count + 1 + return orig_fetch_checker(...) + end +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local healthcheck_manager = require("apisix.healthcheck_manager") + local checks = [[{ + "active":{ + "http_path":"/hello", + "timeout":1, + "type":"http", + "healthy":{ "interval":1, "successes":1 }, + "unhealthy":{ "interval":1, "http_failures":1 } + } + }]] + -- a standalone /upstreams/ object, matching how a real + -- "critical upstream" is configured (apisixUpstreams entries), + -- not an inline route upstream -- an inline upstream's + -- resource_key is the owning route's key ("/routes/"), not + -- "/upstreams/", which is the identifier space + -- ensure_checker/is_resource_probed are scoped to. + assert(t('/apisix/admin/upstreams/ensure-checker-test', ngx.HTTP_PUT, [[{ + "nodes": {"127.0.0.1:1980": 1}, + "type": "roundrobin", + "checks": ]] .. checks .. [[ + }]]) < 300) + assert(t('/apisix/admin/routes/ensure-checker-test', ngx.HTTP_PUT, [[{ + "upstream_id": "ensure-checker-test", + "uri": "/ensure-checker-test" + }]]) < 300) + + -- no request to /ensure-checker-test at all -- fetch_checker() has + -- never been called for this resource by real traffic + local ok, err = healthcheck_manager.ensure_checker("/upstreams/ensure-checker-test") + ngx.say("ensure_checker: ", tostring(ok), " ", tostring(err)) + + ngx.sleep(1.5) -- let timer_create_checker build the checker on its next tick + + ngx.say("resource probed after ensure_checker with zero traffic: ", + tostring(healthcheck_manager.is_resource_probed("/upstreams/ensure-checker-test"))) + } + } +--- request +GET /t +--- response_body_like +ensure_checker: true nil +resource probed after ensure_checker with zero traffic: true +--- timeout: 5 + + + +=== TEST 5: all_targets_probed() with min_attempts > 1 waits for real convergence +# Closes a gap found via live kind testing: a single active-check attempt is +# not enough to know a target's real state when unhealthy.http_failures (or +# .tcp_failures/.timeouts, or healthy.successes) is configured above 1 -- +# internal_health only actually converges after that many CONSECUTIVE +# attempts. A readiness gate that opens after just 1 attempt can still route +# real traffic to a target whose true state has not yet been reached. This +# is why the shm value is now a per-target probe *count* +# (hack/patches/lua-resty-healthcheck-probed-gate.patch), not a boolean, and +# why all_targets_probed() takes an explicit min_attempts argument that +# apisix.healthcheck_manager.is_resource_probed() computes from the +# checker's own thresholds (required_probe_attempts()). +--- config + location /t { + content_by_lua_block { + local healthcheck = require("resty.healthcheck") + + local checker = healthcheck.new({ + name = "test-ps12691-min-attempts", + shm_name = "upstream-healthcheck", + checks = { + active = { + type = "http", + -- a path the mock backend does not serve: 404 is in + -- the default active.unhealthy.http_statuses list, + -- so every attempt fails deterministically without + -- needing a dedicated always-500 mock endpoint + http_path = "/definitely-not-a-real-endpoint-ps12691", + timeout = 1, + healthy = { interval = 1, successes = 1 }, + unhealthy = { interval = 1, http_failures = 2 }, + }, + }, + events_module = "resty.events", + }) + if not checker then + ngx.say("failed to create checker") + return + end + + local ok, err = checker:add_target("127.0.0.1", 1980, nil, true, nil) + if not ok then + ngx.say("failed to add target: ", err) + return + end + ngx.sleep(0.2) -- let add_target's own event settle locally + + ngx.sleep(1.5) -- past one interval: exactly one attempt has fired + + local after_one = healthcheck.all_targets_probed( + "test-ps12691-min-attempts", "upstream-healthcheck", 2) + ngx.say("after 1 attempt, min_attempts=2: ", tostring(after_one)) + + -- min_attempts=1 must already be satisfied after just 1 attempt, + -- proving the gap is specifically about the threshold, not a + -- broken counter. + local after_one_threshold_one = healthcheck.all_targets_probed( + "test-ps12691-min-attempts", "upstream-healthcheck", 1) + ngx.say("after 1 attempt, min_attempts=1: ", tostring(after_one_threshold_one)) + + ngx.sleep(1.2) -- past a second interval: a second attempt has fired + + local after_two = healthcheck.all_targets_probed( + "test-ps12691-min-attempts", "upstream-healthcheck", 2) + ngx.say("after 2 attempts, min_attempts=2: ", tostring(after_two)) + + checker:stop() + } + } +--- request +GET /t +--- response_body +after 1 attempt, min_attempts=2: false +after 1 attempt, min_attempts=1: true +after 2 attempts, min_attempts=2: true +--- no_error_log +[error] +--- timeout: 8