Copilot commented on code in PR #13801:
URL: https://github.com/apache/apisix/pull/13801#discussion_r3782497501
##########
t/plugin/ai-transport-http.t:
##########
@@ -353,3 +353,93 @@ connect: operation timed out => 504
connect: Operation timed out => 504
request: connection refused => 500
request: connection reset by peer => 500
+
+
+
+=== TEST 8: AI transport applies independent connect send and read timeouts
+--- config
+ location /t {
+ content_by_lua_block {
+ local orig_http = package.loaded["resty.http"]
+ local orig_transport =
package.loaded["apisix.plugins.ai-transport.http"]
+
+ package.loaded["resty.http"] = {
+ new = function()
+ return {
+ set_timeout = function()
+ error("legacy set_timeout must not be used")
+ end,
+ set_timeouts = function(_, connect, send, read)
+ ngx.say(connect, ",", send, ",", read)
+ end,
+ connect = function() return true end,
+ request = function() return {headers = {}, status =
200} end,
+ }
+ end,
+ }
+
+ package.loaded["apisix.plugins.ai-transport.http"] = nil
+ local transport = require("apisix.plugins.ai-transport.http")
+ local res, err = transport.request({
+ host = "127.0.0.1",
+ port = 80,
+ path = "/",
+ body = {},
+ }, {
+ connect_timeout = 101,
+ send_timeout = 202,
+ read_timeout = 303,
+ })
+ if not res then
+ ngx.say(err)
+ end
+
+ package.loaded["resty.http"] = orig_http
+ package.loaded["apisix.plugins.ai-transport.http"] = orig_transport
+ }
+ }
+--- response_body
+101,202,303
+
+
+
+=== TEST 9: AI transport keeps numeric timeout callers on set_timeout
+--- config
+ location /t {
+ content_by_lua_block {
+ local orig_http = package.loaded["resty.http"]
+ local orig_transport =
package.loaded["apisix.plugins.ai-transport.http"]
+
+ package.loaded["resty.http"] = {
+ new = function()
+ return {
+ set_timeout = function(_, timeout)
+ ngx.say(timeout)
+ end,
+ set_timeouts = function()
+ error("set_timeouts must not be used for numeric
callers")
+ end,
+ connect = function() return true end,
+ request = function() return {headers = {}, status =
200} end,
+ }
+ end,
+ }
+
+ package.loaded["apisix.plugins.ai-transport.http"] = nil
+ local transport = require("apisix.plugins.ai-transport.http")
+ local res, err = transport.request({
+ host = "127.0.0.1",
+ port = 80,
+ path = "/",
+ body = {},
+ }, 456)
+ if not res then
+ ngx.say(err)
+ end
+
+ package.loaded["resty.http"] = orig_http
+ package.loaded["apisix.plugins.ai-transport.http"] = orig_transport
+ }
Review Comment:
Same module-cache cleanup issue as TEST 8: if `error()` triggers before
lines 440–441, `package.loaded[\"resty.http\"]` stays stubbed for subsequent
tests. Add a guaranteed cleanup path so the restore always runs even on failure.
##########
docs/en/latest/plugins/ai-proxy.md:
##########
@@ -93,7 +93,10 @@ When `provider` is set to `bedrock`, the Plugin expects
requests in the [Bedrock
| logging | object | False | |
| Logging configurations. Does not affect `error.log`. |
| logging.summaries | boolean | False | false |
| If true, logs request LLM model, duration, request, and response
tokens. |
| logging.payloads | boolean | False | false |
| If true, logs request and response payload. |
-| timeout | integer | False | 30000 | 1 - 600000
| Request timeout in milliseconds when requesting the LLM service.
Applied per socket operation (connect / send / read block); does not cap the
total duration of a streaming response. |
+| timeout | integer | False | 30000 | 1 - 600000
| Default request timeout in milliseconds when requesting the LLM
service. It remains the fallback for each phase below and is used for all
phases when no phase-specific timeout is configured. |
Review Comment:
This updated `timeout` description no longer states the important behavior
that the timeout is applied per socket operation (connect/send/read) and does
not cap total streaming duration (previously documented). Since backward
compatibility relies on preserving those semantics, consider reintroducing that
clarification here (and similarly in the other timeout tables), so existing
users don’t misinterpret `timeout` as a wall-clock request deadline.
##########
t/plugin/ai-transport-http.t:
##########
@@ -353,3 +353,93 @@ connect: operation timed out => 504
connect: Operation timed out => 504
request: connection refused => 500
request: connection reset by peer => 500
+
+
+
+=== TEST 8: AI transport applies independent connect send and read timeouts
+--- config
+ location /t {
+ content_by_lua_block {
+ local orig_http = package.loaded["resty.http"]
+ local orig_transport =
package.loaded["apisix.plugins.ai-transport.http"]
+
+ package.loaded["resty.http"] = {
+ new = function()
+ return {
+ set_timeout = function()
+ error("legacy set_timeout must not be used")
+ end,
+ set_timeouts = function(_, connect, send, read)
+ ngx.say(connect, ",", send, ",", read)
+ end,
+ connect = function() return true end,
+ request = function() return {headers = {}, status =
200} end,
+ }
+ end,
+ }
+
+ package.loaded["apisix.plugins.ai-transport.http"] = nil
+ local transport = require("apisix.plugins.ai-transport.http")
+ local res, err = transport.request({
+ host = "127.0.0.1",
+ port = 80,
+ path = "/",
+ body = {},
+ }, {
+ connect_timeout = 101,
+ send_timeout = 202,
+ read_timeout = 303,
+ })
+ if not res then
+ ngx.say(err)
+ end
+
+ package.loaded["resty.http"] = orig_http
+ package.loaded["apisix.plugins.ai-transport.http"] = orig_transport
+ }
Review Comment:
The test mutates `package.loaded` and restores it at the end, but if an
`error()`/assertion occurs before the restore lines (e.g., unexpected
`set_timeout` usage), the worker’s module cache may remain poisoned and cascade
failures into later tests. Wrap the patched section in `pcall`/`xpcall` (or
equivalent) and ensure the restore happens in a guaranteed cleanup path.
##########
apisix/plugins/ai-proxy/schema.lua:
##########
@@ -344,6 +344,21 @@ _M.ai_proxy_schema = {
default = 30000,
description = "timeout in milliseconds",
},
+ connect_timeout = {
+ type = "integer",
+ minimum = 1,
+ maximum = 600000,
+ },
+ send_timeout = {
+ type = "integer",
+ minimum = 1,
+ maximum = 600000,
+ },
+ read_timeout = {
+ type = "integer",
+ minimum = 1,
+ maximum = 600000,
Review Comment:
The new schema properties omit `description` fields, unlike `timeout` (and
other nearby fields). Adding brief descriptions (e.g., connect/send/read phase
semantics + fallback to `timeout`) improves generated schema documentation and
makes validation errors easier to understand.
--
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]
