Skip to content

fix: route LLMaaS health probes through proxy - #31

Merged
chrlesur merged 1 commit into
mainfrom
fix/llmaas-health-proxy
Jul 8, 2026
Merged

fix: route LLMaaS health probes through proxy#31
chrlesur merged 1 commit into
mainfrom
fix/llmaas-health-proxy

Conversation

@aelttil

@aelttil aelttil commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • adapt /health and system_health LLMaaS probes to use PROXY_URL when configured
  • reuse the same explicit httpx.AsyncClient(proxy=...) pattern already used by ConsolidatorService
  • keep the consolidator behavior unchanged
  • add proxy coverage for both HTTP health and MCP system_health

Tests

  • uv run pytest tests/test_proxy.py -q
  • uv run pytest -q

@aelttil
aelttil force-pushed the fix/llmaas-health-proxy branch 2 times, most recently from 2fe38c8 to 04fdd7a Compare July 3, 2026 06:03
@aelttil
aelttil force-pushed the fix/llmaas-health-proxy branch from 04fdd7a to 1b01999 Compare July 3, 2026 06:04

@chrlesur chrlesur 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

Verdict: APPROVE — this fixes a real health-check blind spot: in a proxied environment, the LLMaaS probes went out direct while every real LLM call goes through PROXY_URL, so /health and system_health could report a path that production traffic never takes (false negative — or worse, false positive when only the direct route works).

Checks performed

  • Full suite on the PR branch: 415 passed + 1 xfailed (baseline 413 + the 2 new tests).
  • Pattern is strictly aligned with ConsolidatorService (explicit httpx.AsyncClient(proxy=...) injected as http_client); the explicitly created proxy client is closed in finally — no leak on that path.
  • models.list() result is consumed via .data only (already loaded), so reading it after aclose() is safe — no lazy pagination after close.
  • Error sanitization on the public /health endpoint: already covered by the pre-existing LM2-24 handler (generic "LLMaaS unreachable", details logged server-side only) which wraps this change — no proxy URL/credential can leak in the response. A malformed PROXY_URL is additionally rejected at startup by config validation.
  • No-proxy behavior unchanged (http_client=None → SDK-managed client).
  • The two tests assert the meaningful contract: proxy kwarg + timeout, http_client handed to AsyncOpenAI, aclose awaited, final status.

Optional (no change requested)

  1. This is now the third copy of the proxy+AsyncOpenAI construction pattern (middleware, system tool, consolidator) — a small shared helper would be a nice future factorization, also the right place for an exception-safe close.
  2. Public /health triggers one LLMaaS hit per request; bounded by the global rate limit today. A short-TTL cache would be a future improvement if monitoring traffic ever makes this noisy.

@chrlesur
chrlesur merged commit 56de432 into main Jul 8, 2026
2 checks passed
@chrlesur chrlesur mentioned this pull request Jul 8, 2026
pull Bot pushed a commit to moul/live-memory that referenced this pull request Jul 8, 2026
Bump VERSION, __version__ and version assert to 2.6.0; add CHANGELOG
entry for the health-probe proxy fix (Cloud-Temple#31); document PROXY_URL in
ARCHITECTURE.md (now covers S3, LLMaaS consolidation and health probes);
refresh version badges in both READMEs.

Minor bump: server-side behavior change on the outbound network path.
Consumes the 2.6.0 slot; the graph_push volatile guardrail (Wave B,
PR Cloud-Temple#29) moves to the next minor once its blocking review items land.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants