Skip to content

fix(ai-proxy-multi): keep existing query string in health check path; cover construct_upstream mutation contract#13506

Merged
nic-6443 merged 2 commits into
apache:masterfrom
nic-6443:test/ai-proxy-multi-construct-upstream
Jun 11, 2026
Merged

fix(ai-proxy-multi): keep existing query string in health check path; cover construct_upstream mutation contract#13506
nic-6443 merged 2 commits into
apache:masterfrom
nic-6443:test/ai-proxy-multi-construct-upstream

Conversation

@nic-6443

@nic-6443 nic-6443 commented Jun 10, 2026

Copy link
Copy Markdown
Member

Description

Two related changes to construct_upstream in ai-proxy-multi:

Fix: auth.query broke health check paths that already contain a query string. The code always joined auth.query onto checks.active.http_path with ?, so a configured path like /status?probe=ready became /status?probe=ready?api_key=... and the active probe requested a wrong path (which can flip healthy instances to unhealthy). Now & is used when the path already contains ?.

Test: cover the no-mutation contract. construct_upstream is invoked once per second per instance by the healthcheck manager timers, always against the cached route config. The checks table is cloned before applying auth.header / auth.query; without the clone, the auth query string would be appended to checks.active.http_path again on every call and the cached config would silently diverge from the config center. This contract had no coverage — the new test calls construct_upstream repeatedly and asserts the returned checks stay stable and the input instance table is not mutated.

Both test cases fail without their respective code behavior: TEST 1 fails if the deepcopy is removed (call 2: unexpected http_path: /status?api_key=secret?api_key=secret), TEST 2 fails without the separator fix (call 1: unexpected http_path: /status?probe=ready?api_key=secret).

Which issue(s) this PR fixes:

N/A

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

…checks

construct_upstream is invoked once per second per instance by the
healthcheck manager timers, always against the cached route config. The
checks table is cloned before applying auth header/query; without the
clone, the auth query string would be appended to checks.active.http_path
again on every call and the cached config would no longer match the config
center. Add a regression test asserting the returned checks stay stable
across repeated calls and the input table is not mutated.
Copilot AI review requested due to automatic review settings June 10, 2026 10:14
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…uery to health check path

construct_upstream always joined auth.query to checks.active.http_path
with "?", so a configured path that already carries a query string (e.g.
/status?probe=ready) became /status?probe=ready?api_key=... and the
active probe requested a wrong path. Use "&" when the path already
contains "?".
@nic-6443 nic-6443 changed the title test(ai-proxy-multi): cover construct_upstream not mutating instance checks fix(ai-proxy-multi): keep existing query string in health check path; cover construct_upstream mutation contract Jun 11, 2026
@nic-6443 nic-6443 merged commit cb72d0e into apache:master Jun 11, 2026
20 checks passed
@nic-6443 nic-6443 deleted the test/ai-proxy-multi-construct-upstream branch June 11, 2026 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants