Skip to content

fix(proxy): check x-headroom-base-url off the event loop in chat and responses - #4856

Open
chopratejas wants to merge 8 commits into
mainfrom
fix/upstream-guard-off-loop
Open

chopratejas wants to merge 8 commits into
mainfrom
fix/upstream-guard-off-loop

Conversation

@chopratejas

@chopratejas chopratejas commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Description

handle_openai_chat and handle_openai_responses validated x-headroom-base-url with the synchronous is_safe_upstream_url, which waits for the DNS lookup (up to HEADROOM_UPSTREAM_RESOLVE_TIMEOUT_S, 3 s by default) on the event-loop thread. Chat did this three times per request and responses twice, so one slow-resolving host held up every other in-flight request for several seconds. The other routes in headroom/providers/proxy_routes.py already use is_safe_upstream_url_async.

Both handlers now resolve the header once, through a new _resolve_openai_upstream_base_async that uses the async guard (lookup on a worker thread, pin scope opened on the request's task), and reuse that result for the destination and the extra-header decision.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Performance improvement

Changes Made

  • headroom/proxy/handlers/openai.py: header parsing split into _openai_base_url_candidate; sync _resolve_openai_upstream_base (kept for _resolve_openai_upstream) and new _resolve_openai_upstream_base_async share it; _openai_upstream_from(request, custom) picks the upstream from an already-resolved override; chat and responses call the async resolver once.
  • tests/test_proxy/test_openai_upstream_guard_off_loop.py: asserts the guard runs once and not on the event-loop thread, for chat and responses.
  • tests/test_proxy_openai_cache_key_integration.py: its stub now patches the async resolver the handler calls.

Sibling PRs that touch the same code (merge order note): fix/upstream-idna-pin-once adds the identical _openai_upstream_from and resolve-once hunks (this PR is a superset of that part); fix/openai-base-url-reject changes the rejection branch of _resolve_openai_upstream_base, which after this PR must be mirrored in _resolve_openai_upstream_base_async. Suggested order: idna-pin-once, then this PR, then reject (rebasing reject means raising the 400 from the shared rejection path in both resolvers).

Testing

  • Unit tests pass (pytest)
  • Linting passes (ruff check .)
  • Type checking passes (mypy headroom)
  • New tests added for new functionality
  • Manual testing performed

Test Output

# New test on origin/main (c21b0813)
E   AssertionError: the guard ran on the event-loop thread
E   assert [True, True, True] == [False]      (chat)
E   assert [True, True] == [False]            (responses)
============================== 2 failed in 0.72s ===============================

# This branch: new test + cache-key integration
============================== 7 passed in 7.30s ===============================

# Area suites: test_upstream_guard, test_grok_shared_proxy_call_sites, test_proxy/test_openai_upstream_header,
# test_proxy_openai_cache_key_integration, test_upstream_credential_scoping, test_header_isolation,
# test_proxy_hardening, test_openai_codex_routing, test_proxy/test_openai_upstream_guard_off_loop
======================= 282 passed, 5 warnings in 56.32s =======================

$ ruff check / ruff format --check (0.17.3) on changed files
All checks passed!
3 files already formatted
$ mypy headroom/proxy/handlers/openai.py --ignore-missing-imports
Success: no issues found in 1 source file

Real Behavior Proof

  • Environment: macOS, Python 3.12, full app from create_app driven through httpx.ASGITransport on one event loop; socket.getaddrinfo replaced so slow.example takes 2 s to answer (everything else real).
  • Exact command / steps: start a /v1/chat/completions request with x-headroom-base-url: https://slow.example/v1, then 0.2 s later send GET /livez and time it.
  • Observed result:
    == origin/main
    /livez sent 0.2s after a chat request naming a 2s-to-resolve host: HTTP 200 after 6.03s (0.20s if the loop is free)
    == this branch
    /livez sent 0.2s after a chat request naming a 2s-to-resolve host: HTTP 200 after 0.20s (0.20s if the loop is free)
    
  • Not tested: under real slow DNS.

Runtime Rollout Safety

  • Rollout-managed feature(s): none
  • Minimum rollout channel: n/a
  • Stable/default behavior changed: no change in which upstream is chosen; the lookup moves to a worker thread and happens once per request.
  • Kill switch / disable path: none needed
  • Unsafe override required: no
  • Qualification impact: none
  • Rollback path: revert

Review Readiness

  • I have performed a self-review
  • This PR is ready for human review

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I did not edit CHANGELOG.md — it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this)

Additional Notes

Fix adapted from a proposal by HetCreep.

Follow-up, not in this PR: select_passthrough_base_url (headroom/providers/proxy_targets.py, Azure api-key branch) and headroom/providers/registry.py still call the sync guard; the passthrough route already checks the same header with the async guard first, so the sync call there is a second lookup on the loop.

🤖 Generated with Claude Code


Devin Review

Reported by @JoaoMarcos44. Thank you for your contribution! 🩵

…responses

The chat-completions and responses handlers validated the client-chosen
upstream with the blocking guard, which waits up to the resolve timeout on
the event-loop thread, and did so up to three times per request. A slow
resolving host therefore held up every other in-flight request.

Both handlers now resolve the header once per request through the async
guard, which runs the lookup on a worker thread, and reuse that result for
the destination and the extra-header decision.

Assay-Kind: bugfix
Assay-Ask: The chat and responses handlers wait on DNS for x-headroom-base-url on the event-loop thread, stalling other requests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chopratejas chopratejas added bug Something isn't working size: small At most 100 added/deleted lines. risk: low Scope estimate: documentation/tests-only change; verification still required. area: proxy Touches proxy files; triage routing, not a CODEOWNERS assignment. labels Nov 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR governance

This PR does not yet satisfy the required template fields:

  • Fill in Real Behavior Proof → Observed result.

Please update the PR body, or move the PR back to draft while it is still in progress.

@github-actions github-actions Bot added status: needs author action Pull request body or readiness checklist still needs author updates status: ci failing Required or reported CI checks are failing labels Nov 13, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

… in chat

The chat handler now checks the override once, before image compression.
The guard's pin lasts 60 s, so a compression that ran longer left only an
expired pin and the connection was refused. After image compression runs,
the same URL is checked again to renew the pin; the destination is not
chosen again, and a host that no longer passes is refused with 400.

Assay-Kind: bugfix
Assay-Ask: A chat request with a custom base URL is refused at connect time when image compression outlasts the upstream pin.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the status: ci failing Required or reported CI checks are failing label Nov 13, 2026
@codecov-commenter

codecov-commenter commented Nov 13, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…y tests

Two chat tests replaced the synchronous guard to let deliberately
unresolvable hosts through. The chat handler now awaits the async guard,
so the stub no longer applied and the override was dropped. Stub the
function the handler calls.

Assay-Kind: tests
Assay-Ask: Two chat cache-stability tests stub the sync guard, which the chat handler no longer calls, so their custom hosts are rejected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Assay-Kind: deps
Assay-Ask: The dependency audit checks failed on this PR because the branch predates the dependency updates on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review completed; approval remains pending the items below.

Reviewed head d4eb81585181e5ff5b86f1f7fc0534a5513fb588 against main 0368a872cdd5064b39888e13847200ff562ac192.

Qualification still required:

  • Passing test evidence is missing
  • Exact-source provenance review is incomplete

devin-ai-integration[bot]

This comment was marked as resolved.

… loop

The post-image-compression check of x-headroom-base-url ran on every image
request, even with a still-valid pin, so a failed second lookup could
refuse a valid request; on one branch it also waited for DNS on the event
loop. The check now runs only when this request's pin for that host has
expired, and always through the async guard.

Assay-Kind: bugfix
Assay-Ask: The chat handler re-resolves the custom base URL after every image compression, on the event loop and even when the pin is still valid.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review completed; approval remains pending the items below.

Reviewed head 1416d0d8c5276d1b84c7f09e1facaae97450a714 against main 0368a872cdd5064b39888e13847200ff562ac192.

Qualification still required:

  • Hosted check evidence changed
  • Exact-source provenance review is incomplete
  • Check/run 132232314322 is not terminal
  • Check/run 132232314308 is not terminal
  • Check/run 132232314254 is not terminal
  • Check/run 132232314208 is not terminal
  • Check/run 43525780933 is not terminal

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1416d0d against target 0368a87. The async guard preserves request-scoped pins and routing/header decisions; both image-compression pin findings are resolved. Local validation passed 17 handler/routing cases and 26 cache integration cases (the latter in documented Python-only mode because the worker lacks the native extension). Exact-head hosted CI, including all four test shards, is now successful. No blocking code issues found.

JerrettDavis
JerrettDavis previously approved these changes Nov 14, 2026

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 55828ed against target b324efb. The async guard preserves routing, header trust and request-scoped pins; both prior image-compression pin findings remain resolved. Fresh local validation passed 27 handler/routing cases and 26 ASGI cache cases in documented Python-only mode. The new off-loop regression fails on the recorded base for both handlers and passes on head. Staged exact-head CI is successful, including all four test shards. No blocking code issues found. Native compression and real DNS/provider traffic were not tested locally.

@JerrettDavis JerrettDavis added the size: medium 101–500 added/deleted lines. label Nov 14, 2026
@JerrettDavis JerrettDavis removed the size: small At most 100 added/deleted lines. label Nov 14, 2026
chopratejas and others added 2 commits November 13, 2026 21:19
Conflict-only merge. Kept this branch's async resolver for the chat and
responses handlers and main's lapsed-pin re-check after image compression
(which already uses the async guard); took main's resolve-once test.

Assay-Kind: config
Assay-Ask: The branch conflicts with main after the sibling upstream fix was merged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o fix/upstream-guard-off-loop

Brings in the earlier merge of main already on the branch; no content change.

Assay-Kind: config
Assay-Ask: The branch conflicts with main after the sibling upstream fix was merged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chopratejas

Copy link
Copy Markdown
Collaborator Author

Jerrett: conflict-only merge with main (239f0de, plus 3582001 to take in your earlier merge commit; no content change from that one). Kept this PR's async resolver in the chat and responses handlers, and main's lapsed-pin re-check after image compression, which already goes through the async guard. Took main's resolve-once test. Area tests, ruff and mypy pass locally. Your approval was dismissed by the push, so I re-requested review.

@github-actions github-actions Bot added the status: ci failing Required or reported CI checks are failing label Nov 14, 2026
@chopratejas

Copy link
Copy Markdown
Collaborator Author

The one red check, test (3), is tests/test_memory_tools_only_when_injected.py::test_memory_call_the_proxy_did_not_inject_reaches_the_client[not_injected-anthropic*], which fails the same way on main (fda75de) and passes locally on this branch.

This branch was successfully deployed

1 active deployment
Preview — 35820017 Deployed Nov 14, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: proxy Touches proxy files; triage routing, not a CODEOWNERS assignment. bug Something isn't working risk: low Scope estimate: documentation/tests-only change; verification still required. size: medium 101–500 added/deleted lines. status: ci failing Required or reported CI checks are failing status: needs author action Pull request body or readiness checklist still needs author updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants