From c2e649eb940335efb57652955a7a7fd86c88d569 Mon Sep 17 00:00:00 2001 From: Ricardo Corral Date: Thu, 25 Jun 2026 19:08:50 -0600 Subject: [PATCH 1/2] feat: retire idle keep-alive connections after 2s (PYGQLC_KEEPALIVE_EXPIRY) Pass httpx.Limits(keepalive_expiry=2.0) (down from httpx's 5.0 default, tunable via PYGQLC_KEEPALIVE_EXPIRY) to both the sync and async clients so a socket the server/LB has already closed during an idle gap is never reused -- the stale keep-alive that surfaces as ReadError(''). Standard pool caps (max_connections=100, max_keepalive_connections=20) are kept explicitly, since passing only keepalive_expiry would reset them to unlimited. Pairs with the 3.8.4 fresh-connection retry: fewer stale sockets means the retry -- which is unsafe to apply blindly to non-idempotent mutations and can duplicate writes (e.g. workflowServiceRun, OPS-4976) -- fires far less often. Bumps version to 3.8.5. --- CHANGELOG.md | 4 ++ pygqlc/GraphQLClient.py | 27 ++++++++-- pygqlc/__version__.py | 2 +- .../gql_client/test_keepalive_expiry.py | 53 +++++++++++++++++++ 4 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 tests/pygqlc/gql_client/test_keepalive_expiry.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 27d3a24..d8dffaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # CHANGELOG +## [3.8.5] - 2026-06-26 + +- [Changed] The HTTP clients now retire idle keep-alive connections after 2s (`httpx.Limits(keepalive_expiry=2.0)`, down from httpx's 5.0 default), tunable via the `PYGQLC_KEEPALIVE_EXPIRY` env var. This stops the client from reusing a socket the server/LB has already closed during an idle gap — the stale keep-alive that surfaces as `ReadError('')`. Hot connections under load are reused within milliseconds, so pooling is preserved; only genuinely idle sockets are dropped. Pairs with the 3.8.4 fresh-connection retry: fewer stale sockets to begin with, so the retry (which is unsafe to apply blindly to non-idempotent mutations) fires far less often. + ## [3.8.4] - 2026-06-25 - [Fixed] `async_execute` retries once on a fresh connection for transient transport errors (`httpx.NetworkError`/`ReadError`, `RemoteProtocolError`, `PoolTimeout`, `ConnectTimeout`) — typically a stale keep-alive socket the server closed, surfacing as `ReadError('')`. `ReadTimeout` is excluded (a slow request would just time out again). Sync `execute` already behaved this way. diff --git a/pygqlc/GraphQLClient.py b/pygqlc/GraphQLClient.py index 29d8b51..f14a9ff 100644 --- a/pygqlc/GraphQLClient.py +++ b/pygqlc/GraphQLClient.py @@ -8,6 +8,7 @@ """ import asyncio +import os import traceback import time import threading @@ -50,6 +51,14 @@ httpx.ConnectTimeout, ) +# Drop an idle pooled keep-alive connection after this many seconds so we never +# reuse a socket the server has already closed (the stale keep-alive that +# surfaces as ReadError('')). httpx defaults to 5.0, which is long enough for a +# server/LB with a shorter idle timeout to close the socket first. 2.0 sits below +# any reasonable server idle timeout while still pooling hot connections under +# load (those are reused within milliseconds). Tunable via PYGQLC_KEEPALIVE_EXPIRY. +DEFAULT_KEEPALIVE_EXPIRY = 2.0 + # * Custom Exception class for GraphQL responses @@ -286,9 +295,21 @@ def __init__(self): self.pingIntervalTime = 15 self.pingTimer = time.time() - # Setup common client parameters - self.client_params = {"http2": True} - self.async_client_params = {"http2": True} + # Setup common client parameters. Retire idle keep-alive connections + # quickly (see DEFAULT_KEEPALIVE_EXPIRY) so a socket the server has + # already closed is never reused. + keepalive_expiry = float( + os.environ.get("PYGQLC_KEEPALIVE_EXPIRY", DEFAULT_KEEPALIVE_EXPIRY) + ) + # Keep httpx's standard pool sizing (passing only keepalive_expiry would + # reset both maxes to None / unlimited); just shorten the idle expiry. + limits = httpx.Limits( + max_connections=100, + max_keepalive_connections=20, + keepalive_expiry=keepalive_expiry, + ) + self.client_params = {"http2": True, "limits": limits} + self.async_client_params = {"http2": True, "limits": limits} # Reuse HTTP client for better performance self._http_client = None diff --git a/pygqlc/__version__.py b/pygqlc/__version__.py index 9fd9cda..56d5ad1 100644 --- a/pygqlc/__version__.py +++ b/pygqlc/__version__.py @@ -1 +1 @@ -__version__ = "3.8.4" +__version__ = "3.8.5" diff --git a/tests/pygqlc/gql_client/test_keepalive_expiry.py b/tests/pygqlc/gql_client/test_keepalive_expiry.py new file mode 100644 index 0000000..01ab288 --- /dev/null +++ b/tests/pygqlc/gql_client/test_keepalive_expiry.py @@ -0,0 +1,53 @@ +"""Idle keep-alive connections are retired quickly via httpx ``keepalive_expiry`` +so a socket the server already closed is never reused — the stale keep-alive that +surfaces as ``ReadError('')``. Default 2s, tunable via ``PYGQLC_KEEPALIVE_EXPIRY``. +""" + +import httpx +import pytest + +from pygqlc import GraphQLClient +from pygqlc.GraphQLClient import DEFAULT_KEEPALIVE_EXPIRY +from pygqlc.helper_modules.Singleton import Singleton + + +@pytest.fixture +def fresh_client(): + """Build GraphQLClient instances whose ``__init__`` re-reads the env, without + leaking state into the process-wide singleton other tests share.""" + saved = Singleton._instances.pop(GraphQLClient, None) + + def _build(): + Singleton._instances.pop(GraphQLClient, None) + return GraphQLClient() + + yield _build + + Singleton._instances.pop(GraphQLClient, None) + if saved is not None: + Singleton._instances[GraphQLClient] = saved + + +def test_default_keepalive_expiry_on_both_clients(fresh_client, monkeypatch): + monkeypatch.delenv("PYGQLC_KEEPALIVE_EXPIRY", raising=False) + gql = fresh_client() + for params in (gql.client_params, gql.async_client_params): + limits = params["limits"] + assert isinstance(limits, httpx.Limits) + assert limits.keepalive_expiry == DEFAULT_KEEPALIVE_EXPIRY + # httpx's standard pool caps must be preserved, not reset to None. + assert limits.max_connections == 100 + assert limits.max_keepalive_connections == 20 + + +def test_keepalive_expiry_env_override(fresh_client, monkeypatch): + monkeypatch.setenv("PYGQLC_KEEPALIVE_EXPIRY", "0.5") + gql = fresh_client() + assert gql.client_params["limits"].keepalive_expiry == 0.5 + assert gql.async_client_params["limits"].keepalive_expiry == 0.5 + + +def test_default_below_httpx_default(): + # The point of the mitigation: shorter than httpx's 5.0s default so a + # server/LB with a shorter idle timeout can't close the socket first. + assert DEFAULT_KEEPALIVE_EXPIRY < 5.0 From adb2674d41c059ef0f371fa59eea1757cf5dca8e Mon Sep 17 00:00:00 2001 From: Ricardo Corral Date: Fri, 26 Jun 2026 08:53:02 -0600 Subject: [PATCH 2/2] fix: thread keepalive limits into ipv4_only transport + validate env var Addresses review on #78: - httpx applies the Client-level `limits` only to its default transport; with a custom transport (ipv4_only) the limits kwarg is ignored, so keepalive_expiry was silently dropped there (fell back to httpx's 5s). Stash the limits on self and pass them into HTTPTransport/AsyncHTTPTransport so the mitigation applies on both paths. - PYGQLC_KEEPALIVE_EXPIRY: a non-numeric or negative value now falls back to the default with a warning rather than raising during client construction. - Tests: compare the default against httpx.Limits().keepalive_expiry instead of a hardcoded 5.0 (robust to httpx upgrades); add coverage for the ipv4_only transport limits and the env-validation fallback. --- CHANGELOG.md | 2 +- pygqlc/GraphQLClient.py | 55 +++++++++++++++---- .../gql_client/test_keepalive_expiry.py | 42 +++++++++++++- 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8dffaf..ab69162 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## [3.8.5] - 2026-06-26 -- [Changed] The HTTP clients now retire idle keep-alive connections after 2s (`httpx.Limits(keepalive_expiry=2.0)`, down from httpx's 5.0 default), tunable via the `PYGQLC_KEEPALIVE_EXPIRY` env var. This stops the client from reusing a socket the server/LB has already closed during an idle gap — the stale keep-alive that surfaces as `ReadError('')`. Hot connections under load are reused within milliseconds, so pooling is preserved; only genuinely idle sockets are dropped. Pairs with the 3.8.4 fresh-connection retry: fewer stale sockets to begin with, so the retry (which is unsafe to apply blindly to non-idempotent mutations) fires far less often. +- [Changed] The HTTP clients now retire idle keep-alive connections after 2s (`httpx.Limits(keepalive_expiry=2.0)`, down from httpx's 5.0 default), tunable via the `PYGQLC_KEEPALIVE_EXPIRY` env var (a non-numeric or negative value falls back to the default with a warning). The expiry applies on both the default transport and the `ipv4_only` custom transport — httpx ignores the client-level `limits` when a custom transport is supplied, so the limits are threaded into the transport too. This stops the client from reusing a socket the server/LB has already closed during an idle gap — the stale keep-alive that surfaces as `ReadError('')`. Hot connections under load are reused within milliseconds, so pooling is preserved; only genuinely idle sockets are dropped. Pairs with the 3.8.4 fresh-connection retry: fewer stale sockets to begin with, so the retry (which is unsafe to apply blindly to non-idempotent mutations) fires far less often. ## [3.8.4] - 2026-06-25 diff --git a/pygqlc/GraphQLClient.py b/pygqlc/GraphQLClient.py index f14a9ff..a431fc4 100644 --- a/pygqlc/GraphQLClient.py +++ b/pygqlc/GraphQLClient.py @@ -59,6 +59,36 @@ # load (those are reused within milliseconds). Tunable via PYGQLC_KEEPALIVE_EXPIRY. DEFAULT_KEEPALIVE_EXPIRY = 2.0 + +def _resolve_keepalive_expiry() -> float: + """Idle keep-alive expiry (seconds) from the PYGQLC_KEEPALIVE_EXPIRY env var. + + Falls back to DEFAULT_KEEPALIVE_EXPIRY (with a WARNING) on a non-numeric or + negative value, so a misconfigured env var degrades to the safe default + instead of crashing GraphQLClient construction (and the worker) at startup. + """ + raw = os.environ.get("PYGQLC_KEEPALIVE_EXPIRY") + if raw is None: + return DEFAULT_KEEPALIVE_EXPIRY + try: + value = float(raw) + except ValueError: + log( + LogLevel.WARNING, + f"Invalid PYGQLC_KEEPALIVE_EXPIRY={raw!r} (not a number); " + f"using default {DEFAULT_KEEPALIVE_EXPIRY}s", + ) + return DEFAULT_KEEPALIVE_EXPIRY + if value < 0: + log( + LogLevel.WARNING, + f"Invalid PYGQLC_KEEPALIVE_EXPIRY={raw!r} (negative); " + f"using default {DEFAULT_KEEPALIVE_EXPIRY}s", + ) + return DEFAULT_KEEPALIVE_EXPIRY + return value + + # * Custom Exception class for GraphQL responses @@ -297,19 +327,18 @@ def __init__(self): # Setup common client parameters. Retire idle keep-alive connections # quickly (see DEFAULT_KEEPALIVE_EXPIRY) so a socket the server has - # already closed is never reused. - keepalive_expiry = float( - os.environ.get("PYGQLC_KEEPALIVE_EXPIRY", DEFAULT_KEEPALIVE_EXPIRY) - ) + # already closed is never reused. Stashed on self so _update_client_params + # can thread the same limits into a custom transport (ipv4_only), where + # httpx ignores the Client-level `limits` kwarg. # Keep httpx's standard pool sizing (passing only keepalive_expiry would # reset both maxes to None / unlimited); just shorten the idle expiry. - limits = httpx.Limits( + self._limits = httpx.Limits( max_connections=100, max_keepalive_connections=20, - keepalive_expiry=keepalive_expiry, + keepalive_expiry=_resolve_keepalive_expiry(), ) - self.client_params = {"http2": True, "limits": limits} - self.async_client_params = {"http2": True, "limits": limits} + self.client_params = {"http2": True, "limits": self._limits} + self.async_client_params = {"http2": True, "limits": self._limits} # Reuse HTTP client for better performance self._http_client = None @@ -984,11 +1013,17 @@ def _update_client_params(self, ipv4_only): # Binding the local egress address to 0.0.0.0 forces httpx to use an # IPv4 source socket (the standard idiom for IPv4-only egress). This is # not a listening socket, so the bind-all-interfaces concern does not apply. + # httpx applies the Client-level `limits` only to its *default* + # transport; with a custom transport the limits kwarg is ignored, so + # pass our limits (keepalive_expiry) into the transport itself or the + # mitigation is silently dropped on this path. self.client_params["transport"] = httpx.HTTPTransport( - local_address="0.0.0.0" # nosec B104 + local_address="0.0.0.0", # nosec B104 + limits=self._limits, ) self.async_client_params["transport"] = httpx.AsyncHTTPTransport( - local_address="0.0.0.0" # nosec B104 + local_address="0.0.0.0", # nosec B104 + limits=self._limits, ) else: # Remove transport if it exists diff --git a/tests/pygqlc/gql_client/test_keepalive_expiry.py b/tests/pygqlc/gql_client/test_keepalive_expiry.py index 01ab288..2179ef5 100644 --- a/tests/pygqlc/gql_client/test_keepalive_expiry.py +++ b/tests/pygqlc/gql_client/test_keepalive_expiry.py @@ -48,6 +48,44 @@ def test_keepalive_expiry_env_override(fresh_client, monkeypatch): def test_default_below_httpx_default(): - # The point of the mitigation: shorter than httpx's 5.0s default so a + # The point of the mitigation: shorter than httpx's own default so a # server/LB with a shorter idle timeout can't close the socket first. - assert DEFAULT_KEEPALIVE_EXPIRY < 5.0 + # Compare against the live httpx default so this doesn't go stale on upgrades. + assert DEFAULT_KEEPALIVE_EXPIRY < httpx.Limits().keepalive_expiry + + +def test_invalid_env_falls_back_to_default(fresh_client, monkeypatch): + monkeypatch.setenv("PYGQLC_KEEPALIVE_EXPIRY", "not-a-number") + gql = fresh_client() + assert gql.client_params["limits"].keepalive_expiry == DEFAULT_KEEPALIVE_EXPIRY + + +def test_negative_env_falls_back_to_default(fresh_client, monkeypatch): + monkeypatch.setenv("PYGQLC_KEEPALIVE_EXPIRY", "-5") + gql = fresh_client() + assert gql.client_params["limits"].keepalive_expiry == DEFAULT_KEEPALIVE_EXPIRY + + +def test_ipv4_only_threads_limits_into_transports(fresh_client, monkeypatch): + """In ipv4_only mode a custom transport is installed; httpx ignores the + Client-level limits there, so the limits must be passed into the transport.""" + gql = fresh_client() + captured = {} + + def fake_http_transport(**kwargs): + captured["sync"] = kwargs + return object() + + def fake_async_transport(**kwargs): + captured["async"] = kwargs + return object() + + monkeypatch.setattr(httpx, "HTTPTransport", fake_http_transport) + monkeypatch.setattr(httpx, "AsyncHTTPTransport", fake_async_transport) + + gql._update_client_params(ipv4_only=True) + + assert captured["sync"]["limits"] is gql._limits + assert captured["async"]["limits"] is gql._limits + assert captured["sync"]["local_address"] == "0.0.0.0" + assert captured["async"]["local_address"] == "0.0.0.0"