fix: stop PulseAgent blocking the hypercode-core event loop - #311
Conversation
PulseAgent.process() is a coroutine running on hypercode-core's event loop, and it called requests.get() -- the sync api -- to query Prometheus with a 5s timeout. A stall there delays every request the core API is serving, not just the pulse call. Replace with an awaited httpx.AsyncClient. The client is built ONCE at module import, not per call, and that detail is the whole fix. Constructing an AsyncClient builds an SSL context and loads the CA bundle: hundreds of milliseconds of synchronous work. The obvious rewrite -- `async with httpx.AsyncClient() as c` inside process() -- measured WORSE than the bug it replaced. Worst event-loop stall per call, measured inside the running container with a 50ms ticker: requests.get (sync) 53 ms AsyncClient built per call 284 ms <- worse than the bug shared AsyncClient (shipped) 8 ms Verified against the shipped module (import app.agents.pulse; drive its real _http client): steady-state stall 1-10ms across runs 2-4, status 200, 14 Prometheus targets parsed. First call in a fresh process costs ~126ms of one-time transport warmup, then settles. httpx.Response exposes .status_code/.json()/.text identically to requests, so the parsing below the call is untouched. pulse.py was the only user of `requests` in backend/app. Not exercised: brain.think() below the fetch. That path is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughPulseAgent's Prometheus data fetch in ChangesPulse Agent Async Migration
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant PulseAgent
participant AsyncHttpClient
participant Prometheus
PulseAgent->>AsyncHttpClient: await _http.get(prometheus up query)
AsyncHttpClient->>Prometheus: GET request
Prometheus-->>AsyncHttpClient: JSON response
AsyncHttpClient-->>PulseAgent: response.json()
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)backend/app/agents/pulse.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/app/agents/pulse.py`:
- Line 14: The shared module-level AsyncClient in pulse.py is never closed, so
its connection pool can leak on shutdown. Add a dedicated close helper for the
_http client (for example, a close_http_client function near the existing client
setup) that awaits _http.aclose(), and make sure the application shutdown path
or lifespan handler invokes it. Keep the client reuse pattern, but wire its
lifecycle to startup/shutdown so the singleton client is disposed cleanly.
- Line 14: The module-level `_http` `httpx.AsyncClient` in `pulse.py` is created
once but never closed, so its connection pool can remain open for the process
lifetime. Update the app shutdown/lifespan teardown path to call
`_http.aclose()` explicitly, or wire `_http` into the existing lifespan cleanup
alongside the other app resources so it is always closed when the app stops.
- Around line 2-15: The pulse agent switched from requests to a shared
httpx.AsyncClient, so the existing tests that patch pulse_mod.requests.get will
fail because requests no longer exists and the new path uses _http.get. Update
the unit tests in test_agent_pulse.py to mock pulse_mod._http.get (or otherwise
inject a client) so they intercept the awaited HTTP call in pulse.process/brain
usage and avoid AttributeError during collection.
🪄 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: ac4939b7-b64e-407f-92f2-c6689793e7b2
📒 Files selected for processing (1)
backend/app/agents/pulse.py
| import httpx | ||
| from app.agents.brain import brain | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Built once, at import. Constructing an AsyncClient builds an SSL context and | ||
| # loads the CA bundle — hundreds of ms of *synchronous* work. Doing that inside | ||
| # process() stalls the loop harder than the sync requests.get it replaced. | ||
| # Measured in-container, worst loop stall per call: | ||
| # requests.get (sync) 53 ms | ||
| # AsyncClient built per call 284 ms <- worse than the bug | ||
| # shared AsyncClient (this) 8 ms | ||
| _http = httpx.AsyncClient(timeout=5.0) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unit tests will break: they still mock pulse_mod.requests.get.
The removal of import requests (replaced by import httpx) means pulse_mod.requests no longer exists. All three tests in backend/tests/unit/test_agent_pulse.py call monkeypatch.setattr(pulse_mod.requests, "get", fake_get), which will raise AttributeError at collection time. Additionally, even if patched, the mock would not intercept the new await _http.get(...) path.
The tests must be migrated to mock _http.get (or inject a client) instead.
🔧 Example test migration pattern
# In test_agent_pulse.py
- def fake_get(url: str, params: dict, timeout: int):
- return _Response(200, {...})
-
- monkeypatch.setattr(pulse_mod.requests, "get", fake_get)
+ async def fake_get(url, params=None):
+ return _Response(200, {...})
+
+ monkeypatch.setattr(pulse_mod._http, "get", fake_get)🤖 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 `@backend/app/agents/pulse.py` around lines 2 - 15, The pulse agent switched
from requests to a shared httpx.AsyncClient, so the existing tests that patch
pulse_mod.requests.get will fail because requests no longer exists and the new
path uses _http.get. Update the unit tests in test_agent_pulse.py to mock
pulse_mod._http.get (or otherwise inject a client) so they intercept the awaited
HTTP call in pulse.process/brain usage and avoid AttributeError during
collection.
| # requests.get (sync) 53 ms | ||
| # AsyncClient built per call 284 ms <- worse than the bug | ||
| # shared AsyncClient (this) 8 ms | ||
| _http = httpx.AsyncClient(timeout=5.0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Module-level AsyncClient is never closed — connection pool leaks on shutdown.
The shared _http client is intentionally long-lived, but there is no aclose() call anywhere. On process restart or graceful shutdown, the underlying connection pool and SSL context are not disposed, which can leak sockets and file descriptors.
Consider wiring an aclose() into the application's lifespan/shutdown handler, or use a pattern that ties the client lifecycle to the app:
♻️ Suggested lifecycle management
_http = httpx.AsyncClient(timeout=5.0)
+
+async def close_http_client() -> None:
+ """Call during application shutdown."""
+ await _http.aclose()Then invoke close_http_client() from the app's shutdown signal handler (e.g., FastAPI lifespan or equivalent).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _http = httpx.AsyncClient(timeout=5.0) | |
| _http = httpx.AsyncClient(timeout=5.0) | |
| async def close_http_client() -> None: | |
| """Call during application shutdown.""" | |
| await _http.aclose() |
🤖 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 `@backend/app/agents/pulse.py` at line 14, The shared module-level AsyncClient
in pulse.py is never closed, so its connection pool can leak on shutdown. Add a
dedicated close helper for the _http client (for example, a close_http_client
function near the existing client setup) that awaits _http.aclose(), and make
sure the application shutdown path or lifespan handler invokes it. Keep the
client reuse pattern, but wire its lifecycle to startup/shutdown so the
singleton client is disposed cleanly.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is it safe to create httpx.AsyncClient at module import time (outside an event loop) and use it later inside an async context in httpx 0.28.1?
💡 Result:
It is generally unsafe and not recommended to create an httpx.AsyncClient at module import time. While initializing the AsyncClient object itself does not immediately require an active event loop, the client's internal mechanisms (such as connection pooling and transport initialization) are designed to operate within an asynchronous context. Creating it at the module level introduces several risks: 1. Lifecycle Management: The AsyncClient is intended to be used as a context manager (using async with) or explicitly closed using await client.aclose() to ensure resources like connection pools are properly released [1][2]. Module-level objects persist for the lifetime of the application, making it difficult to ensure they are shut down gracefully, which can lead to resource leaks and warnings about unclosed clients [3]. 2. Event Loop Binding: While newer versions of httpx have improved compatibility, many asynchronous libraries in Python bind resources to the event loop that is active when the resource is first used or initialized. Creating the client outside of an async context may lead to issues if the client inadvertently attempts to bind to an event loop that is not yet started or is not the intended loop for its operations. 3. Testing and Best Practices: Best practice dictates that the AsyncClient should be instantiated within the lifecycle of your application's event loop—typically within an async function, a startup/shutdown handler (if using a framework like FastAPI), or a fixture (if using pytest-asyncio). This ensures the client is tied to a valid event loop and is correctly closed after use [3][2]. In summary, to avoid unexpected runtime errors, resource leakage, and event loop incompatibility, you should initialize your httpx.AsyncClient inside an async function or within an appropriate lifecycle hook, rather than at the module level [3][2].
Citations:
- 1: https://github.com/encode/httpx/blob/master/httpx/_client.py
- 2: https://github.com/encode/httpx/blob/master/docs/advanced/clients.md
- 3: asynclient not closing with aclose : UserWarning: Unclosed <httpx.AsyncClient object at 0x000001EEBB46E520>. encode/httpx#1851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and locate any AsyncClient lifecycle handling.
git ls-files 'backend/app/agents/pulse.py' 'backend/app/**' | sed -n '1,20p'
printf '\n--- pulse.py ---\n'
cat -n backend/app/agents/pulse.py
printf '\n--- AsyncClient references ---\n'
rg -n "AsyncClient|aclose\(|async with .*httpx|httpx" backend/app -SRepository: welshDog/HyperCode-V2.4
Length of output: 10815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- references to backend/app/agents/pulse.py symbols ---'
rg -n "\bpulse\b|\b_http\b|PulseAgent" backend/app -S
printf '\n%s\n' '--- main.py shutdown section ---'
cat -n backend/app/main.py | sed -n '180,250p'
printf '\n%s\n' '--- pulse.py surrounding context ---'
cat -n backend/app/agents/pulse.pyRepository: welshDog/HyperCode-V2.4
Length of output: 8713
Close the shared AsyncClient on shutdown. The module-level client is fine to construct, but it is never aclose()d, so the connection pool stays open for the process lifetime. Wire it into app lifespan teardown or close it explicitly.
🤖 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 `@backend/app/agents/pulse.py` at line 14, The module-level `_http`
`httpx.AsyncClient` in `pulse.py` is created once but never closed, so its
connection pool can remain open for the process lifetime. Update the app
shutdown/lifespan teardown path to call `_http.aclose()` explicitly, or wire
`_http` into the existing lifespan cleanup alongside the other app resources so
it is always closed when the app stops.
1 participant
Third and last of today's sync-I/O-on-an-async-loop fixes (see #309, #310). This is the only one in a container that's actually running.
What's wrong
PulseAgent.process()(backend/app/agents/pulse.py:20) is a coroutine onhypercode-core's event loop, and it queried Prometheus with synchronousrequests.get(timeout=5). A stall there delays every request the core API is serving, not just the pulse call.pulse.pywas the only user ofrequestsanywhere inbackend/app.The part that isn't obvious
The textbook rewrite —
async with httpx.AsyncClient() as c: await c.get(...)insideprocess()— measured worse than the bug.Constructing an
AsyncClientbuilds an SSL context and loads the CA bundle: hundreds of milliseconds of synchronous work, executed on the loop, on every call.Worst event-loop stall per call, measured inside the running container with a 50ms ticker task:
requests.get(the bug)AsyncClientbuilt per callAsyncClient, built onceSo the client is built once at module import. That detail is the fix; awaiting alone would have made things worse.
Verification
Driven against the shipped module in the rebuilt container — importing
app.agents.pulseand exercising its real_httpclient, not a copy of the code:Steady state is 1–10ms. The first call in a fresh process costs ~126ms of one-time transport warmup, then settles — that happens once per process, not per call.
hypercode-corerebuilt,healthy, 0 restarts,/health→{"status":"ok","version":"2.4.2"}.httpx.Responseexposes.status_code,.json(), and.textidentically torequests, so the parsing below the call is untouched.Not verified
brain.think()below the fetch is unchanged and was not exercised — it calls the LLM.Context
Found by an AST sweep for sync I/O lexically inside
async defbodies, after a manual audit missed it. The sweep also flaggedagents/hypervisor-agent/hypervisor_agent.py:450,495andservices/agent-spawner/spawner.py:117,187— both real, neither deployed (no containers), both deliberately left alone here.Worth recording the sweep's blind spot: it only catches calls written lexically inside a coroutine. It would not have caught the
broski-pets-bridgebug in #310, where the sync work sits in a helperdefthat anasync defcalls. A clean AST run means clean of the lexical form, not clean.🤖 Generated with Claude Code
Summary by CodeRabbit