feat: add FFI functions to populate upstream state variables#112
Merged
Conversation
Add push_upstream_state() and update_upstream_state() C FFI functions that populate r->upstream_states, enabling standard nginx upstream variables ($upstream_status, $upstream_addr, $upstream_response_time, etc.) to be set from Lua when the upstream module is bypassed (e.g., ai-proxy cosocket). 2-step API: - push: creates a new upstream state entry with addr/status/connect_time/header_time - update: fills in final response_time/response_length on the last entry Unset timing fields default to -1 (renders as "-" in logs, not "0.000"). Each retry attempt can push a separate entry, matching nginx multi-value semantics (e.g., "502, 200" in $upstream_status).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two Lua APIs—push_upstream_state(opts) and update_upstream_state(opts)—and corresponding C functions that push and update per-request upstream state. New TAP tests validate variable exposure, sentinel defaults, multi-record sequences, and error behavior when updating without a prior push. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Lua as lib/resty/apisix/upstream.lua
participant FFI_C as C (ngx_http_apisix_module)
participant Request as ngx_http_request_t
participant NginxVars as nginx vars / readers
Client->>Lua: call push_upstream_state(opts)
Lua->>Request: get_request()
Lua->>FFI_C: C.ngx_http_apisix_push_upstream_state(r, addr?, addr_len, status, connect_ms, header_ms)
FFI_C->>Request: ensure r->upstream_states, push new upstream state, copy addr/status, set timings if >= 0
Request->>NginxVars: expose upstream_status / upstream_addr / timing fields
Client->>Lua: call update_upstream_state(opts)
Lua->>FFI_C: C.ngx_http_apisix_update_upstream_state(r, response_ms, response_len)
FFI_C->>Request: update last upstream state response_time/response_length when >= 0
Request->>NginxVars: upstream_response_time / upstream_response_length updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds C FFI + Lua wrapper APIs to manually populate r->upstream_states for cosocket-based upstream requests, so standard nginx $upstream_* variables become available for access logs and Lua-variable reads even when the upstream module is bypassed.
Changes:
- Add
ngx_http_apisix_push_upstream_state()/ngx_http_apisix_update_upstream_state()to create/update entries inr->upstream_states. - Expose Lua APIs
push_upstream_state()/update_upstream_state()vialib/resty/apisix/upstream.lua. - Add
t/upstream_state.tcoverage for status/addr, timings, dash rendering, retry semantics, and error path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| t/upstream_state.t | Adds integration tests validating $upstream_* variable population and formatting. |
| src/ngx_http_apisix_module.h | Declares new C functions for pushing/updating upstream state. |
| src/ngx_http_apisix_module.c | Implements upstream state creation/update logic against r->upstream_states. |
| lib/resty/apisix/upstream.lua | Adds LuaJIT FFI bindings and Lua-facing helper functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
log_format directive is only valid in http context, not location. Rewrite tests to verify upstream variables via ngx.var reads.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
t/upstream_state.t (1)
123-142: Optional: assert return values in retry test for better failure diagnostics.This block currently ignores
ok, err; checking them would make failures easier to pinpoint when regressions occur.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/upstream_state.t` around lines 123 - 142, The test currently ignores the return values from upstream.push_upstream_state and upstream.update_upstream_state; update the retry test to capture and assert the returned ok, err (or equivalent) for each call to upstream.push_upstream_state and upstream.update_upstream_state so failures show precise diagnostics—e.g., assign results from upstream.push_upstream_state({ ... }) and assert ok is truthy (and err is nil or contains expected text), and do the same for each upstream.update_upstream_state call for both the failed and succeeding attempts.lib/resty/apisix/upstream.lua (1)
134-171: Add defensiveoptsvalidation to avoid hard runtime exceptions.Line 140 and Line 164 index
optsdirectly; passingnil/non-table will throw instead of returning a controlled API error.Proposed hardening
function _M.push_upstream_state(opts) + if type(opts) ~= "table" then + return nil, "opts expects a table" + end + local r = get_request() if not r then return nil, "no request found" @@ function _M.update_upstream_state(opts) + if type(opts) ~= "table" then + return nil, "opts expects a table" + end + local r = get_request() if not r then return nil, "no request found"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resty/apisix/upstream.lua` around lines 134 - 171, Both _M.push_upstream_state and _M.update_upstream_state assume opts is a table and index into it directly; add a defensive check at the start of each function to validate opts is a table (or at least non-nil) and return nil plus a clear error string when it's invalid, so accessing opts.addr, opts.status, opts.connect_time, opts.header_time, opts.response_time and opts.response_length cannot raise a runtime error; update the functions to perform that check before calling get_request() or before using any opts fields and keep existing return values/NGX_ERROR handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resty/apisix/upstream.lua`:
- Around line 16-27: The FFI cdef currently hardcodes "typedef long off_t" and
uses ngx_int_t for millisecond time params, which mismatches the C API; update
the cdef so off_t is defined in a platform-aware way (or removed to match the
system typedef) instead of forcing "long", and change the time parameter types
from ngx_int_t to ngx_msec_int_t for the functions
ngx_http_apisix_push_upstream_state (connect_time_ms, header_time_ms) and
ngx_http_apisix_update_upstream_state (response_time_ms); keep all other
signatures unchanged so the Lua FFI aligns exactly with the C header types.
---
Nitpick comments:
In `@lib/resty/apisix/upstream.lua`:
- Around line 134-171: Both _M.push_upstream_state and _M.update_upstream_state
assume opts is a table and index into it directly; add a defensive check at the
start of each function to validate opts is a table (or at least non-nil) and
return nil plus a clear error string when it's invalid, so accessing opts.addr,
opts.status, opts.connect_time, opts.header_time, opts.response_time and
opts.response_length cannot raise a runtime error; update the functions to
perform that check before calling get_request() or before using any opts fields
and keep existing return values/NGX_ERROR handling unchanged.
In `@t/upstream_state.t`:
- Around line 123-142: The test currently ignores the return values from
upstream.push_upstream_state and upstream.update_upstream_state; update the
retry test to capture and assert the returned ok, err (or equivalent) for each
call to upstream.push_upstream_state and upstream.update_upstream_state so
failures show precise diagnostics—e.g., assign results from
upstream.push_upstream_state({ ... }) and assert ok is truthy (and err is nil or
contains expected text), and do the same for each upstream.update_upstream_state
call for both the failed and succeeding attempts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06839f44-2568-4a9f-bb5d-a725846b4607
📒 Files selected for processing (4)
lib/resty/apisix/upstream.luasrc/ngx_http_apisix_module.csrc/ngx_http_apisix_module.ht/upstream_state.t
- Remove duplicate off_t typedef (conflicts with client.lua), use intptr_t - Add input validation for push/update_upstream_state opts parameter
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- response_length is off_t, not ngx_msec_t; -1 renders as '-1' not '-' Match nginx behavior: leave at 0 from memzero - Remove resty.ngxvar dependency from TEST 7 (not in CI env) - Fix TEST 3 expected: response_length shows '0' when unset
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resty/apisix/upstream.lua`:
- Around line 143-150: Validate and sanitize option field types before calling
the FFI functions to avoid LuaJIT FFI argument errors: ensure opts.addr is
either nil or a string (get its length only after confirming string), and ensure
numeric fields (opts.status, opts.connect_time, opts.header_time, and the other
numeric field used in the later FFI call at the 169-172 block) are numbers
(coerce with tonumber or return nil, err if invalid) before invoking
C.ngx_http_apisix_push_upstream_state (and the similar FFI call in the 169-172
block); return nil, err on invalid types instead of passing bad values into the
FFI call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e424d64e-daa3-49bc-a992-366cfce40abc
📒 Files selected for processing (1)
lib/resty/apisix/upstream.lua
There was a problem hiding this comment.
🧹 Nitpick comments (1)
t/upstream_state.t (1)
122-157: Capture and assertok, errfor push/update in these tests.These blocks call
push_upstream_state/update_upstream_statewithout checking return values. Adding explicit checks makes failures more actionable and avoids partial silent behavior.Also applies to: 180-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/upstream_state.t` around lines 122 - 157, The tests call push_upstream_state and update_upstream_state without checking their return values; modify each invocation to capture the returned ok, err (e.g., local ok, err = upstream.push_upstream_state(...)) and immediately assert success (assert(ok, err) or the test framework's assert) so failures surface with the error message; apply the same change for all occurrences (including the other block around lines 180-210) referencing push_upstream_state and update_upstream_state to ensure each call's result is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@t/upstream_state.t`:
- Around line 122-157: The tests call push_upstream_state and
update_upstream_state without checking their return values; modify each
invocation to capture the returned ok, err (e.g., local ok, err =
upstream.push_upstream_state(...)) and immediately assert success (assert(ok,
err) or the test framework's assert) so failures surface with the error message;
apply the same change for all occurrences (including the other block around
lines 180-210) referencing push_upstream_state and update_upstream_state to
ensure each call's result is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f56693e-2285-4e69-b9f7-cc96da6a5745
📒 Files selected for processing (2)
src/ngx_http_apisix_module.ct/upstream_state.t
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/resty/apisix/upstream.lua (1)
148-153:⚠️ Potential issue | 🟠 MajorValidate numeric option fields before FFI invocation.
Line 151–153 and Line 174–175 pass user-provided values directly to FFI. Non-numeric inputs can raise LuaJIT conversion exceptions and bypass the intended
nil, errcontract.In LuaJIT FFI, what happens when a C function parameter is `int`/`intptr_t` and the caller passes a Lua string or table value?💡 Proposed fix
+local function validate_number_field(v, name) + if v ~= nil and type(v) ~= "number" then + return nil, name .. " must be a number" + end + return true +end + function _M.push_upstream_state(opts) @@ local addr = opts.addr if addr ~= nil and type(addr) ~= "string" then return nil, "addr must be a string" end + local ok, err = validate_number_field(opts.status, "status") + if not ok then + return nil, err + end + ok, err = validate_number_field(opts.connect_time, "connect_time") + if not ok then + return nil, err + end + ok, err = validate_number_field(opts.header_time, "header_time") + if not ok then + return nil, err + end + local addr_len = addr and `#addr` or 0 @@ function _M.update_upstream_state(opts) @@ + local ok, err = validate_number_field(opts.response_time, "response_time") + if not ok then + return nil, err + end + ok, err = validate_number_field(opts.response_length, "response_length") + if not ok then + return nil, err + end + local ret = C.ngx_http_apisix_update_upstream_state( r, opts.response_time or -1,Also applies to: 172-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resty/apisix/upstream.lua` around lines 148 - 153, Validate and sanitize numeric fields before calling the FFI functions: for the C.ngx_http_apisix_push_upstream_state invocation(s) (and the other similar call at the later lines), coerce opts.status, opts.connect_time, opts.header_time (and any other numeric opts passed into C.*) with tonumber and explicitly handle non-numeric values by returning nil, "invalid numeric field: <field_name>" (or a similar error) instead of passing them directly to FFI; if tonumber returns nil use the documented default (-1 or 0 as appropriate) only when the option is absent, otherwise treat an explicit non-numeric value as an error so LuaJIT conversion exceptions are avoided and the nil, err contract is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/resty/apisix/upstream.lua`:
- Around line 148-153: Validate and sanitize numeric fields before calling the
FFI functions: for the C.ngx_http_apisix_push_upstream_state invocation(s) (and
the other similar call at the later lines), coerce opts.status,
opts.connect_time, opts.header_time (and any other numeric opts passed into C.*)
with tonumber and explicitly handle non-numeric values by returning nil,
"invalid numeric field: <field_name>" (or a similar error) instead of passing
them directly to FFI; if tonumber returns nil use the documented default (-1 or
0 as appropriate) only when the option is absent, otherwise treat an explicit
non-numeric value as an error so LuaJIT conversion exceptions are avoided and
the nil, err contract is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3980bdc9-2c06-40ce-89cd-4e910dca0187
📒 Files selected for processing (1)
lib/resty/apisix/upstream.lua
- Add TEST 8: cosocket HTTP request to mock upstream + access_log verification via regex (mirrors ai-proxy's real usage pattern) - Switch CI from legacy build-apisix-base.sh to production runtime build scripts: build-apisix-runtime.sh (OR 1.27) and build-api7ee-runtime.sh (OR 1.21) - Replace OR version matrix with runtime type matrix matching the two production runtimes (apisix-runtime and api7ee-runtime)
jarvis9443
changed the title
nic-6443
reviewed
Apr 28, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
t/pipe.t TEST 8 and t/upstream_pass_trailers.t rely on nginx features not available in OpenResty 1.21.4.4 (nginx 1.21). These are pre-existing incompatibilities, not caused by this PR.
nic-6443
approved these changes
Apr 28, 2026
membphis
approved these changes
Apr 28, 2026
This was referenced Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add C FFI functions (
push_upstream_state/update_upstream_state) that allow Lua code to populater->upstream_states, making nginx upstream variables ($upstream_status,$upstream_addr,$upstream_response_time,$upstream_header_time,$upstream_connect_time,$upstream_response_length) available in access logs when the upstream module is bypassed (e.g., when using cosocket transport).Why
When plugins like ai-proxy use cosocket (
resty.http) instead ofproxy_pass, nginx upstream variables are empty in access logs becauser->upstream_statesis never populated.API
2-step design:
push_upstream_state(opts)— called when headers arrive or on error"502, 200"semantics)update_upstream_state(opts)— called after body is fully consumedChanges
src/ngx_http_apisix_module.c/.h: C FFI functionslib/resty/apisix/upstream.lua: Lua FFI wrappers with input validationt/upstream_state.t: 8 test cases (unit via ngx.var + integration via access_log).github/workflows/ci.yml: Switch from legacybuild-apisix-base.shto production runtime build scripts (build-apisix-runtime.shfor OR 1.27,build-api7ee-runtime.shfor OR 1.21)