Copilot commented on code in PR #13565:
URL: https://github.com/apache/apisix/pull/13565#discussion_r3419660955
##########
apisix/plugins/ai-proxy-multi.lua:
##########
@@ -569,11 +569,17 @@ local function retry_on_error(ctx, conf, code)
end
end
+ local failed_instance = ctx.picked_ai_instance_name
local name, ai_instance, err = pick_ai_instance(ctx, conf)
if err then
core.log.error("failed to pick new AI instance: ", err)
return 502
end
+ -- The failed attempt's body never reaches the client (a later attempt
+ -- responds instead), so surface the upstream error here for
diagnostics.
+ core.log.warn("ai instance ", failed_instance, " returned status ",
code,
+ ", falling back to ", name, ". upstream error body: ",
+ body or "")
Review Comment:
Logging the full upstream error body may leak sensitive information into
logs (and can also bloat logs if the provider returns a large payload). A safer
approach is to truncate to a small maximum size, normalize/escape newlines,
and/or log structured metadata (e.g., body length + a truncated preview) rather
than the full body.
##########
apisix/plugins/ai-proxy/base.lua:
##########
@@ -298,15 +313,24 @@ function _M.before_proxy(conf, ctx, on_error)
core.response.set_response_source(ctx, "upstream")
if res.status == 429 or (res.status >= 500 and res.status < 600)
then
+ -- Read the upstream error body before closing so the
provider's
+ -- error details survive: logged on fallback (see
retry_on_error)
+ -- and returned to the client when no retry happens.
+ local error_body = read_upstream_error_body(res)
+ local content_type = res.headers["Content-Type"]
+ if content_type then
+ core.response.set_header("Content-Type", content_type)
+ end
Review Comment:
Setting the response `Content-Type` inside the upstream-error branch can
leak the failed attempt’s content-type into a later retry attempt. If
`on_error` triggers a retry (returns nil), the request continues looping but
the header set here may persist; subsequent failures like transport errors may
return without overriding `Content-Type`, resulting in an incorrect header to
the client. Consider deferring `Content-Type` setting until it’s known the
error will be returned to the client (e.g., store it on `ctx`/return it
alongside the body and apply only when not retrying), or explicitly clear/reset
the header before the next attempt.
##########
t/plugin/ai-proxy.t:
##########
@@ -1263,3 +1263,65 @@ got token usage from ai service:
}
--- response_body
OK: auth.query is clean
+
+
+
+=== TEST 38: set route to an upstream that returns 5xx with an error body
Review Comment:
There are multiple consecutive blank lines before the new test block; please
reduce to the project’s typical spacing to keep the test file consistent and
easier to scan.
##########
t/plugin/ai-proxy.t:
##########
@@ -1263,3 +1263,65 @@ got token usage from ai service:
}
--- response_body
OK: auth.query is clean
+
+
+
+=== TEST 38: set route to an upstream that returns 5xx with an error body
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/anything",
+ "plugins": {
+ "ai-proxy": {
+ "provider": "openai-compatible",
+ "auth": {
+ "header": {
+ "Authorization": "Bearer token"
+ }
+ },
+ "options": {
+ "model": "custom"
+ },
+ "override": {
+ "endpoint":
"http://127.0.0.1:6725/v1/chat/completions";
+ },
+ "ssl_verify": false
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 39: single-instance ai-proxy returns the upstream 5xx error body to
the client
+--- http_config
+ server {
+ server_name internal_error;
+ listen 6725;
+ default_type 'application/json';
+ location / {
+ content_by_lua_block {
+ ngx.status = 500
+ ngx.say([[{ "error": {"message":"upstream boom"}}]])
+ return
+ }
+ }
+ }
+--- request
+POST /anything
+{ "messages": [ { "role": "user", "content": "What is 1+1?"} ] }
+--- error_code: 500
+--- response_body_like: upstream boom
Review Comment:
This test validates that the upstream error body is returned, but it doesn’t
assert the PR’s other key promise: preserving the upstream `Content-Type` on
error responses. Add an assertion for the response header (e.g., `Content-Type:
application/json`) so regressions in header propagation are caught.
##########
apisix/plugins/ai-proxy/base.lua:
##########
@@ -298,15 +313,24 @@ function _M.before_proxy(conf, ctx, on_error)
core.response.set_response_source(ctx, "upstream")
if res.status == 429 or (res.status >= 500 and res.status < 600)
then
+ -- Read the upstream error body before closing so the
provider's
+ -- error details survive: logged on fallback (see
retry_on_error)
+ -- and returned to the client when no retry happens.
+ local error_body = read_upstream_error_body(res)
+ local content_type = res.headers["Content-Type"]
+ if content_type then
+ core.response.set_header("Content-Type", content_type)
+ end
if res._t0 then
apisix_upstream.update_upstream_state({
response_time = (ngx_now() - res._t0) * 1000,
+ response_length = res._upstream_bytes or 0,
})
end
if res._httpc then
res._httpc:close()
end
- return res.status
+ return res.status, error_body
Review Comment:
Setting the response `Content-Type` inside the upstream-error branch can
leak the failed attempt’s content-type into a later retry attempt. If
`on_error` triggers a retry (returns nil), the request continues looping but
the header set here may persist; subsequent failures like transport errors may
return without overriding `Content-Type`, resulting in an incorrect header to
the client. Consider deferring `Content-Type` setting until it’s known the
error will be returned to the client (e.g., store it on `ctx`/return it
alongside the body and apply only when not retrying), or explicitly clear/reset
the header before the next attempt.
--
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]
