feat: support OpenResty 1.29.2 patch#20
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds conditional multi-upstream support to nginx 1.29.2 by extending core data structures (connection, request, upstream, stream session) with queue and state-tracking fields, integrating multi-upstream module entry points throughout the HTTP upstream request/response lifecycle, and updating the patch script to recognize and apply the patch to OpenResty 1.29.2 bundles. Changesnginx 1.29.2 multi-upstream integration
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds OpenResty 1.29.2.x support by mapping it to an nginx 1.29.2 patch, keeping this module buildable when bundled with newer OpenResty (e.g., in apisix-runtime).
Changes:
- Extend
patch.shto detectopenresty-1.29.2.*and apply the corresponding nginx patch. - Add
nginx-1.29.2.patchto injectT_NGX_MULTI_UPSTREAM-guarded multi-upstream hooks/struct fields into the nginx 1.29.2 sources.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| patch.sh | Adds OpenResty 1.29.2 version detection and selects the new patch + bundle dir. |
| nginx-1.29.2.patch | New nginx patch enabling multi-upstream integration points for nginx 1.29.2 used by OpenResty 1.29.2.x. |
Comments suppressed due to low confidence (1)
nginx-1.29.2.patch:104
- Log message grammar is unclear: "upstream not support" is ungrammatical and may confuse users. Consider rephrasing to "upstream does not support multi" / "upstream is not configured for multi" so the operator action is unambiguous.
+ NGX_HTTP_INTERNAL_SERVER_ERROR);
+ ngx_log_error(NGX_LOG_ERR, c->log, 0,
+ "multi: need multi, but upstream not support, "
+ "maybe need configuration 'multi' in upstream");
+ return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nginx-1.29.2.patch`:
- Around line 200-207: The current log call inside the
ngx_http_upstream_finalize_request path logs fake-request finalization at
NGX_LOG_ERR and produces false errors; change the log level to a non-error level
(e.g., NGX_LOG_INFO or NGX_LOG_DEBUG) or conditionally log NGX_LOG_ERR only when
rc indicates a real failure (check rc against NGX_OK/NGX_ERROR as appropriate)
in the block that checks u->multi and ngx_http_multi_connection_fake(r) before
calling ngx_http_multi_upstream_finalize_request(r->connection, rc); keep the
same message and parameters but use the lower log level or conditional to avoid
spurious error entries.
- Around line 81-97: When rc == NGX_DONE and ngx_multi_connected(c) returns
false the code currently only logs and returns, leaving r unattached and
hanging; change this branch to explicitly finalize the request instead of
returning. Specifically, in the else block under "if (rc == NGX_DONE)" call
ngx_http_multi_upstream_finalize_request(c, NGX_HTTP_INTERNAL_SERVER_ERROR) (or
another appropriate failure code) and log the failure, rather than just
ngx_log_error then return; do not alter the existing success path that calls
ngx_http_multi_upstream_init_request(c, r) and
ngx_http_multi_upstream_process(c, 1).
🪄 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: 6f3ac23d-53d9-4c44-bb95-aa525f1b0f7b
📒 Files selected for processing (2)
nginx-1.29.2.patchpatch.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nginx-1.29.2.patch (1)
65-98:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix potential use-after-close: log
c->logbeforengx_http_multi_upstream_finalize_request(c, ...)
ngx_http_multi_upstream_finalize_request()detaches and then callsngx_http_multi_upstream_connection_close(c), which destroysc->pooland callsngx_close_connection(c). Logging after it (usingc->log/c) can touch an invalidc.Suggested reordering
if (!(u->multi_mode & NGX_MULTI_UPS_SUPPORT_MULTI)) { + ngx_log_error(NGX_LOG_ERR, c->log, 0, + "multi: upstream configured multi, but handler does not support it"); ngx_http_multi_upstream_finalize_request(c, NGX_HTTP_INTERNAL_SERVER_ERROR); - ngx_log_error(NGX_LOG_ERR, c->log, 0, - "multi: upstream configured multi, but handler does not support it"); return; }} else { + ngx_log_error(NGX_LOG_ERR, c->log, 0, + "multi: connect return %i error", rc); ngx_http_multi_upstream_finalize_request(c, NGX_HTTP_INTERNAL_SERVER_ERROR); - ngx_log_error(NGX_LOG_ERR, c->log, 0, - "multi: connect return %i error", rc); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nginx-1.29.2.patch` around lines 65 - 98, The code logs using c and c->log after calling ngx_http_multi_upstream_finalize_request(c, ...) which can free c (via ngx_http_multi_upstream_connection_close and ngx_close_connection); move any ngx_log_error calls that reference c or c->log to occur before ngx_http_multi_upstream_finalize_request is invoked (and do the same for the branch that calls ngx_http_upstream_finalize_request(r, u, ...)), ensuring you log the error context first, then call ngx_http_multi_upstream_finalize_request or ngx_http_upstream_finalize_request so no logging touches a potentially freed connection object; locate these changes around the ngx_http_multi_upstream_* handlers and branches that check rc, ngx_multi_connected, and the error branches.
🧹 Nitpick comments (1)
nginx-1.29.2.patch (1)
159-163: 💤 Low valueMinor: Inconsistent preprocessor guard style.
Line 159 uses
#if T_NGX_MULTI_UPSTREAMwithout parentheses, while all other occurrences in this patch use#if (T_NGX_MULTI_UPSTREAM)with parentheses.Consistent style
-#if T_NGX_MULTI_UPSTREAM +#if (T_NGX_MULTI_UPSTREAM)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nginx-1.29.2.patch` around lines 159 - 163, The preprocessor guard around the multi-upstream cleanup is inconsistent; change the directive `#if T_NGX_MULTI_UPSTREAM` to use the same parenthesized style `#if (T_NGX_MULTI_UPSTREAM)` so it matches other guards in the patch; keep the inner logic unchanged (the `if (u->multi && rc == NGX_OK) { ngx_multi_clean_leak(u->peer.connection); }` block and the `#endif`) and ensure spacing/indentation follows surrounding code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nginx-1.29.2.patch`:
- Around line 65-98: The code logs using c and c->log after calling
ngx_http_multi_upstream_finalize_request(c, ...) which can free c (via
ngx_http_multi_upstream_connection_close and ngx_close_connection); move any
ngx_log_error calls that reference c or c->log to occur before
ngx_http_multi_upstream_finalize_request is invoked (and do the same for the
branch that calls ngx_http_upstream_finalize_request(r, u, ...)), ensuring you
log the error context first, then call ngx_http_multi_upstream_finalize_request
or ngx_http_upstream_finalize_request so no logging touches a potentially freed
connection object; locate these changes around the ngx_http_multi_upstream_*
handlers and branches that check rc, ngx_multi_connected, and the error
branches.
---
Nitpick comments:
In `@nginx-1.29.2.patch`:
- Around line 159-163: The preprocessor guard around the multi-upstream cleanup
is inconsistent; change the directive `#if T_NGX_MULTI_UPSTREAM` to use the same
parenthesized style `#if (T_NGX_MULTI_UPSTREAM)` so it matches other guards in
the patch; keep the inner logic unchanged (the `if (u->multi && rc == NGX_OK) {
ngx_multi_clean_leak(u->peer.connection); }` block and the `#endif`) and ensure
spacing/indentation follows surrounding code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c35c8e09-b6bc-4d9e-b132-3e948c4d8d8d
📒 Files selected for processing (2)
nginx-1.29.2.patchpatch.sh
nic-6443
approved these changes
May 22, 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.
Adds the nginx 1.29.2 patch mapping used by OpenResty 1.29.2.x so the module can be built by apisix-runtime based on OpenResty 1.29.2.
Validation:
git diff --checkbash -n patch.sh./patch.shto OpenResty 1.29.2.4 sourceSummary by CodeRabbit
New Features
Chores