Skip to content

fix(signal): close httpx client on health check failure to prevent connection leak - #3881

Closed
binhnt92 wants to merge 1 commit into
NousResearch:mainfrom
binhnt92:fix/signal-client-leak-on-health-check
Closed

fix(signal): close httpx client on health check failure to prevent connection leak#3881
binhnt92 wants to merge 1 commit into
NousResearch:mainfrom
binhnt92:fix/signal-client-leak-on-health-check

Conversation

@binhnt92

Copy link
Copy Markdown
Contributor

When connect() creates an httpx.AsyncClient and the subsequent health check fails — either a non-200 response or a network error — both return False paths exit without closing the client. The TCP connection pool, SSL context, and file descriptors are leaked. On repeated reconnect attempts (e.g. the gateway retrying after a daemon restart), this accumulates open connections that are never cleaned up.

Changes Made

Added await self.client.aclose() and self.client = None on both failure paths in the health check block (gateway/platforms/signal.py).

How to Test

python3 -m pytest tests/gateway/test_signal_client_leak.py -v

4 tests: client closed on non-200, client closed on connection error, client kept on success, source verification.

Checklist

  • Tests added (4 tests)
  • Full test suite run — no regressions
  • Tested on Linux (Ubuntu 22.04)

…nnection leak

When connect() creates an httpx.AsyncClient and the subsequent health
check fails (non-200 or network error), both return-False paths exit
without closing the client. The TCP connection pool, SSL context, and
file descriptors are leaked. On repeated reconnect attempts this
accumulates.

Close the client and set it to None on both failure paths.
@dieutx

dieutx commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

I'm working on Signal resource management in #3692 (FIFO eviction for sent-timestamp tracking). The httpx client teardown here looks solid — just want to note that if the health check fires while a message is being sent and tracked in _recent_sent_timestamps, closing the client mid-flight could cause the send to fail silently, and the timestamp would still be added to the tracking set. Might be worth checking if there's an in-flight send before closing.

@binhnt92

Copy link
Copy Markdown
Contributor Author

Looks superseded by #5519 now. Ok to close this one.

@binhnt92 binhnt92 closed this Apr 15, 2026
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