Copilot commented on code in PR #13592: URL: https://github.com/apache/apisix/pull/13592#discussion_r3456944226
########## t/plugin/ai-proxy-multi-construct-upstream-panic.t: ########## @@ -0,0 +1,291 @@ +# +# 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'; + +log_level("info"); +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + + my $user_yaml_config = <<_EOC_; +plugins: + - ai-proxy-multi +_EOC_ + $block->set_value("extra_yaml_config", $user_yaml_config); + + my $http_config = $block->http_config // <<_EOC_; + server { + server_name openai; + listen 127.0.0.1:16724; + + default_type 'application/json'; + + location /v1/chat/completions { + content_by_lua_block { + ngx.say([[{"choices":[{"message":{"content":"ok","role":"assistant"}}]}]]) + } + } + + location /status { + content_by_lua_block { + ngx.say("ok") + } + } + } +_EOC_ + + $block->set_value("http_config", $http_config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: panic in construct_upstream during checker creation must not crash the timer +# construct_upstream is overridden to raise a Lua error on every call. The +# healthcheck manager creation timer wraps it in pcall, logs the failure and +# skips the bad resource instead of aborting checker creation for the whole +# worker. Proxying is unaffected. +--- extra_init_worker_by_lua + local plugin = require "apisix.plugins.ai-proxy-multi" + plugin.construct_upstream = function(instance) + local panic_check + panic_check.cnt = 1 + end Review Comment: The test induces a panic via a nil-index (`panic_check.cnt = 1`), which couples the expected error text to Lua’s runtime wording. Using an explicit `error(\"...\")` would make the intent clearer and the assertion less brittle across Lua/OpenResty versions while still exercising the `pcall` guard path. ########## apisix/healthcheck_manager.lua: ########## @@ -247,17 +254,32 @@ local function timer_working_pool_check() --- callback construct_upstream to create an upstream dynamically local upstream_constructor_config = jp.value(res_conf.value, json_path) local plugin = require("apisix.plugins." .. plugin_name) - upstream = plugin.construct_upstream(upstream_constructor_config) - upstream.resource_key = resource_path + ok, upstream, err = pcall(plugin.construct_upstream, upstream_constructor_config) + if not ok or not upstream then + err = err or upstream + upstream = nil + local err_msg = "[checking checker] unable to construct upstream for plugin: " + .. plugin_name .. ", resource path: " .. resource_path + .. ", json path: " .. json_path .. ", error: " .. err + if not ok then + core.log.error(err_msg) + else + core.log.warn(err_msg) + end + else + upstream.resource_key = resource_path + end else upstream = res_conf.value.upstream or res_conf.value end - local current_ver = upstream_utils.version(res_conf.modifiedIndex, - upstream._nodes_ver) - core.log.info("checking working pool for resource: ", resource_path, - " current version: ", current_ver, " item version: ", item.version) - if item.version == current_ver then - need_destroy = false + if upstream then + local current_ver = upstream_utils.version(res_conf.modifiedIndex, + upstream._nodes_ver) + core.log.info("checking working pool for resource: ", resource_path, + " current version: ", current_ver, " item version: ", item.version) + if item.version == current_ver then + need_destroy = false + end end Review Comment: On `construct_upstream` failure in `timer_working_pool_check`, `upstream` is set to `nil`, so this version check is skipped and `need_destroy` remains `true`. That will destroy the existing working-pool checker entry, which contradicts the PR description (“keeps the existing checker” on refresh path) and reduces resilience (transient failure would remove a valid checker). Consider setting `need_destroy = false` when `construct_upstream` fails so the existing checker is retained until a successful refresh can compute a new version. ########## t/plugin/ai-proxy-multi-construct-upstream-panic.t: ########## @@ -0,0 +1,291 @@ +# +# 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'; + +log_level("info"); +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + + my $user_yaml_config = <<_EOC_; +plugins: + - ai-proxy-multi +_EOC_ + $block->set_value("extra_yaml_config", $user_yaml_config); + + my $http_config = $block->http_config // <<_EOC_; + server { + server_name openai; + listen 127.0.0.1:16724; + + default_type 'application/json'; + + location /v1/chat/completions { + content_by_lua_block { + ngx.say([[{"choices":[{"message":{"content":"ok","role":"assistant"}}]}]]) + } + } + + location /status { + content_by_lua_block { + ngx.say("ok") + } + } + } +_EOC_ + + $block->set_value("http_config", $http_config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: panic in construct_upstream during checker creation must not crash the timer Review Comment: The added tests cover the thrown-error (`pcall` not-ok) path, but the original reported normal failure mode is also `construct_upstream` returning `nil, err` (ok=true but upstream=nil). Consider adding a test case that forces `construct_upstream` to return `nil, \"some error\"` (and optionally `nil` only) to ensure both timers handle that path without crashing, and that the working-pool path retains the existing checker as intended. ########## apisix/healthcheck_manager.lua: ########## @@ -247,17 +254,32 @@ local function timer_working_pool_check() --- callback construct_upstream to create an upstream dynamically local upstream_constructor_config = jp.value(res_conf.value, json_path) local plugin = require("apisix.plugins." .. plugin_name) - upstream = plugin.construct_upstream(upstream_constructor_config) - upstream.resource_key = resource_path + ok, upstream, err = pcall(plugin.construct_upstream, upstream_constructor_config) + if not ok or not upstream then + err = err or upstream + upstream = nil + local err_msg = "[checking checker] unable to construct upstream for plugin: " + .. plugin_name .. ", resource path: " .. resource_path + .. ", json path: " .. json_path .. ", error: " .. err + if not ok then + core.log.error(err_msg) + else + core.log.warn(err_msg) + end + else + upstream.resource_key = resource_path + end Review Comment: In the failure branch, `err` can still be `nil` (e.g., `construct_upstream()` returns only `nil`), and `.. err` will throw (`attempt to concatenate a nil value`) inside the timer—reintroducing the same class of timer-crash this PR is trying to prevent. Fix by ensuring the logged error is always a string (e.g., `tostring(err or upstream or \"unknown\")`) and/or avoid string concatenation by using `core.log.*` with arg lists (as done in `timer_create_checker`). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
