Skip to content

fix: disable httpx proxy for Photon sidecar localhost connections - #47933

Closed
DI404N wants to merge 1 commit into
NousResearch:mainfrom
DI404N:fix/photon-trust-env-localhost
Closed

fix: disable httpx proxy for Photon sidecar localhost connections#47933
DI404N wants to merge 1 commit into
NousResearch:mainfrom
DI404N:fix/photon-trust-env-localhost

Conversation

@DI404N

@DI404N DI404N commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

All Photon sidecar HTTP requests target 127.0.0.1 — they should never be routed through a system HTTP proxy. When trust_env=True (the default), httpx picks up macOS system proxy settings and routes localhost requests through the proxy.

Bug

If the system proxy returns a spurious response (e.g. 502), _reap_stale_sidecar() interprets it as "port in use by a non-sidecar process" and refuses to start the sidecar:

failed to start Photon sidecar: port 8789 is in use by another process
(pids: unknown, not a Photon sidecar)

The proxy intercepts the /healthz POST and returns 502 → code falls through to _find_listener_pids()lsof correctly finds nothing → "pids: unknown" error.

Fix

Set trust_env=False on all five httpx.AsyncClient(...) call sites in the Photon adapter so localhost sidecar communication bypasses the system proxy entirely.

Reproduced & Tested

  • macOS with system HTTP proxy at 127.0.0.1:7890
  • trust_env=False resolves the issue — Photon connects immediately

All Photon sidecar HTTP requests target 127.0.0.1 — they should
never be routed through a system HTTP proxy. When trust_env=True
(the default), httpx picks up macOS system proxy settings and
routes localhost requests through the proxy. If the proxy returns
a spurious response (e.g. 502), _reap_stale_sidecar() interprets
it as 'port in use by a non-sidecar process' and refuses to start,
yielding: 'pids: unknown, not a Photon sidecar'.

Set trust_env=False on all five httpx.AsyncClient call sites in
the Photon adapter so localhost sidecar communication bypasses
the system proxy entirely.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have labels Jun 17, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the focused Photon-sidecar fix. Current main still leaves all five loopback httpx.AsyncClient constructors without an explicit environment-proxy opt-out (plugins/platforms/photon/adapter.py:429, :872, :987, :1550, :1690), and the stale-sidecar probe at :872-886 can treat a non-transport response as an occupied port.

Problems

  • The PR adds no regression test for trust_env=False. The existing reaper fake accepts arbitrary constructor kwargs without inspecting them (tests/plugins/platforms/photon/test_sidecar_lifecycle.py:28-49), so it would not prevent this behavior from regressing.

Suggested changes

  • Add a focused constructor-kwargs capture test covering the loopback sidecar clients, at minimum _reap_stale_sidecar and _standalone_send, and assert trust_env is False.

The five-hunk patch applies cleanly to current main (git apply --check). This is an automated hermes-sweeper review.

@@ -667,7 +667,7 @@ async def _reap_stale_sidecar(self) -> None:
if sys.platform == "win32": # lsof/ps; orphaning is a POSIX-only path

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a regression test that captures the AsyncClient constructor kwargs and asserts trust_env=False for this stale-sidecar health probe; the existing fake client accepts arbitrary kwargs and would not detect a future regression.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #73562 — cherry-picked with authorship preserved and widened to all five sidecar-localhost httpx sites on current main (three grew after your PR). Thanks for the tight diagnosis.

@teknium1 teknium1 closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants