Copilot commented on code in PR #12968:
URL: https://github.com/apache/apisix/pull/12968#discussion_r2802820380
##
apisix/plugins/ai-proxy-multi.lua:
##
@@ -35,7 +35,7 @@ local endpoint_regex = "^(https?)://([^:/]+):?(%d*)/?.*$"
local pickers = {}
Review Comment:
The TTL reduction from 300 to 10 seconds significantly increases SHM access
frequency. Consider documenting why this aggressive TTL is needed, especially
since status_ver changes should already invalidate the cache when health states
change.
```suggestion
local pickers = {}
-- NOTE:
-- The TTL here is intentionally kept small (10s) instead of the more
typical 300s.
-- Health state changes are already handled via status_ver-based
invalidation in the
-- healthcheck manager, but this cache also needs to reflect other dynamic
changes
-- (for example, configuration updates, priority/weight adjustments, or
backend
-- endpoint rotation) in a timely manner for AI traffic routing.
--
-- Using a 10s TTL trades slightly higher SHM access frequency for faster
convergence
-- towards the latest upstream selection state, which is acceptable for this
plugin's
-- usage pattern. If you change this value, consider the impact on both
performance
-- and how quickly picker state reflects configuration and routing changes.
```
##
apisix/plugins/ai-proxy-multi.lua:
##
@@ -229,32 +225,75 @@ local function get_checkers_status_ver(checkers)
end
+-- Read health status directly from SHM to get consistent state across all
workers
+-- This bypasses the local cache which can be stale due to async worker events
+local function fetch_health_status_from_shm(shm, checker_name, ip, port,
hostname, instance_name)
+local lookup_hostname = hostname or ip
+local state_key = string.format("lua-resty-healthcheck:%s:state:%s:%s:%s",
+checker_name,
+ip,
+port,
+lookup_hostname)
+
+core.log.info("[SHM-DIRECT] instance=", instance_name, " key=", state_key)
+local state = shm:get(state_key)
+if state then
+-- State values: 1=healthy, 2=unhealthy, 3=mostly_healthy,
4=mostly_unhealthy
+local ok = (state == 1 or state == 3)
+core.log.info("[SHM-DIRECT] instance=", instance_name, " state=",
state, " ok=", ok)
+return ok, state
+end
+
+-- State not found in SHM (checker not yet created), default to healthy
+core.log.warn("[SHM-DIRECT] state not found for instance=", instance_name,
", defaulting to healthy")
+return true, nil
Review Comment:
Defaulting to healthy when state is not found contradicts the stated goal of
excluding unhealthy instances. This could allow requests to uninitialized
instances. Consider defaulting to unhealthy until the first health check
completes, or document why healthy is the correct default.
```suggestion
-- State not found in SHM (checker not yet created), default to
unhealthy to avoid routing to uninitialized instances
core.log.warn("[SHM-DIRECT] state not found for instance=",
instance_name, ", defaulting to unhealthy")
return false, nil
```
##
apisix/plugins/ai-proxy-multi.lua:
##
@@ -302,23 +341,30 @@ end
function _M.construct_upstream(instance)
local upstream = {}
local node = instance._dns_value
+
+-- If _dns_value doesn't exist (e.g., when called from timer),
+-- calculate it from the instance config
if not node then
-return nil, "failed to resolve endpoint for instance: " ..
instance.name
+core.log.info("instance._dns_value not found, calculating from config
for instance: ", instance.name)
+node = calculate_dns_node(instance)
+if not node then
+return nil, "failed to calculate endpoint for instance: " ..
instance.name
+end
end
if not node.host or not node.port then
return nil, "invalid upstream node: " .. core.json.encode(node)
end
-local node = {
+local upstream_node = {
Review Comment:
The variable name `upstream_node` shadows the outer scope variable `node`.
While functionally correct, this could be confusing. Consider renaming to
something like `node_config` to distinguish the constructed upstream node from
the DNS-resolved node.
##
apisix/plugins/ai-proxy-multi.lua:
##
@@ -177,7 +170,9 @@ local function parse_domain_for_node(node)
end
-local function resolve_endpoint(instance_conf)
+-- Calculate DNS node from instance config without modifying the input
+-- Returns a node table with host, port, scheme fields
Review Comment:
The function comment should clarify that this is intended for use when
_dns_value is not available (e.g., when called from timer context), as this is
a critical use case mentioned in the PR description.
```suggestion
-- Calculate DNS node from instance config without modifying the input.
-- Intended for use when _dns_value i
