feat: retire idle keep-alive connections after 2s to avoid stale-socket ReadError (OPS-4976)

[doctorcorral]

What

Sets httpx.Limits(keepalive_expiry=2.0) (down from httpx's 5.0 default, tunable via PYGQLC_KEEPALIVE_EXPIRY) on both the sync and async clients, so an idle pooled connection is dropped before a server/LB with a shorter idle timeout closes it. Keeps the standard pool caps (max_connections=100, max_keepalive_connections=20) explicitly — passing only keepalive_expiry would reset them to unlimited.

Why

The recurring ReadError('') is a stale keep-alive socket: the server closes an idle connection, the client reuses it, and reading the response fails. Retiring idle sockets after 2s (hot connections under load are reused within ms, so pooling is preserved) means we stop handing back already-closed sockets.

This pairs with the 3.8.4 fresh-connection retry. That retry papered over the symptom but is unsafe to apply blindly to non-idempotent mutations: a ReadError fires while reading the response, so the server may have already committed the write, and the re-POST duplicates it. That's the root of OPS-4976 (duplicate workflowServiceRun rows wedging valiotworkflows state runs on femsa-prod). Fewer stale sockets ⇒ the retry fires far less often. The durable guard for the duplicate-write race is a unique constraint on the Jobs side (separate PR).

Tests

• tests/pygqlc/gql_client/test_keepalive_expiry.py: default 2s on both clients, env override via PYGQLC_KEEPALIVE_EXPIRY, pool caps preserved (100/20), and default < httpx's 5s.
• Full gql_client transient-retry suite still green.

Version → 3.8.5. Part of OPS-4976.

[linear-code]

OPS-4976

[Copilot]

Pull request overview

This PR mitigates stale keep-alive socket reuse (surfacing as httpx.ReadError('')) by shortening the HTTP connection pool’s idle keep-alive expiry to 2 seconds (configurable via PYGQLC_KEEPALIVE_EXPIRY) across both sync and async httpx clients, and ships the corresponding release bump and changelog entry.

Changes:

• Configure httpx.Limits(keepalive_expiry=...) with preserved pool caps (100/20) for both sync and async clients.
• Add tests asserting the default, env override behavior, and that pool caps remain intact.
• Bump version to 3.8.5 and document the behavior change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
pygqlc/GraphQLClient.py	Introduces DEFAULT_KEEPALIVE_EXPIRY, reads PYGQLC_KEEPALIVE_EXPIRY, and applies httpx.Limits to client params.
tests/pygqlc/gql_client/test_keepalive_expiry.py	Adds coverage for default/override keepalive expiry and confirms pool caps are preserved.
pygqlc/__version__.py	Bumps package version to 3.8.5.
CHANGELOG.md	Adds a 3.8.5 entry describing the keepalive expiry change and rationale.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[JoaquinTripp]

keepalive_expiry is silently dropped on the ipv4_only code path

Solid fix — the stale-keep-alive root cause and keeping the pool caps explicit (since Limits(keepalive_expiry=...) alone resets max_connections/max_keepalive_connections to None) are both spot on. One gap worth closing:

When ipv4_only=True, _update_client_params installs a custom transport:

self.client_params["transport"] = httpx.HTTPTransport(local_address="0.0.0.0")
self.async_client_params["transport"] = httpx.AsyncHTTPTransport(local_address="0.0.0.0")

httpx applies the Client-level limits only to its default transport. When a custom transport= is supplied, the limits kwarg is ignored, so on ipv4_only environments the new keepalive_expiry=2.0 never takes effect — the pool falls back to httpx's 5.0s default and the stale-socket mitigation is bypassed exactly where the custom transport runs.

Verified on httpx 0.28.1:

Client(limits), no transport       -> pool keepalive_expiry = 2.0   ✅
Client(limits, transport=custom)   -> transport keepalive    = 5.0   ❌ (limits ignored)
HTTPTransport(limits=limits)       -> keepalive               = 2.0   ✅

Suggested fix — thread limits into the transports (e.g. stash it as self._limits in __init__, since _update_client_params is a separate method):

self.client_params["transport"] = httpx.HTTPTransport(
    local_address="0.0.0.0", limits=self._limits
)
self.async_client_params["transport"] = httpx.AsyncHTTPTransport(
    local_address="0.0.0.0", limits=self._limits
)

Non-blocking for deployments that don't set ipv4_only, but worth closing so the mitigation is uniform across both code paths.

—
Reviewed with Claude Code.

[palantir-valiot]

Overall Assessment

This PR adds httpx.Limits(keepalive_expiry=2.0) (tunable via PYGQLC_KEEPALIVE_EXPIRY) to both the sync and async client parameter dicts, and threads the same limits object into custom HTTPTransport/AsyncHTTPTransport instances when ipv4_only=True. The goal is to drop idle keep-alive sockets before a server/LB can close them, reducing ReadError('') occurrences. Version bumped to 3.8.5, changelog updated, and a dedicated test file added. Implementation is sound; no blocking bugs found.

Findings

No actionable findings.

Notes

• The new test_keepalive_expiry.py follows the repo's TDD/hermetic conventions: it resets the Singleton between tests, uses monkeypatch for env isolation, and avoids real sockets or sleeps.
• The docstring on _resolve_keepalive_expiry and the __init__ comment block explicitly document why pool caps are preserved and why the fallback-with-warning behavior exists; both were considered but not flagged.
• The ipv4_only transport test uses lightweight monkey-patching of httpx.HTTPTransport/AsyncHTTPTransport to assert that limits=self._limits is passed through, which is the minimal integration test the design requires.

[doctorcorral]

Thanks @JoaquinTripp and Copilot — all addressed in adb2674.

🔴 keepalive_expiry silently dropped on the ipv4_only path (JoaquinTripp + Copilot) — fixed. Confirmed exactly as described: with a custom transport=, httpx ignores the Client-level limits. The limits are now stashed as self._limits in __init__ and threaded into both HTTPTransport/AsyncHTTPTransport, so the 2s expiry applies uniformly. Verified on httpx 0.28.1 (real path, no mocks) + a unit test asserting the limits reach both transports.

🟡 Env var parsed without validation (Copilot) — fixed. _resolve_keepalive_expiry() now falls back to the default (with a WARNING) on a non-numeric or negative value, so a typo'd tuning knob degrades gracefully instead of taking down construction. Added tests for both fallbacks. (Kept 0 valid — httpx treats it as "don't reuse idle sockets".)

🟡 Test hardcodes httpx's 5.0 default (Copilot) — fixed. Now asserts DEFAULT_KEEPALIVE_EXPIRY < httpx.Limits().keepalive_expiry, so it won't go stale on an httpx upgrade.

No version change — corrections within the same unreleased 3.8.5.

[palantir-valiot]

Overall Assessment

This PR adds httpx.Limits(keepalive_expiry=2.0) (tunable via PYGQLC_KEEPALIVE_EXPIRY) to both sync and async HTTP clients, threading the same Limits object into custom ipv4_only transports. The implementation is sound: pool caps are preserved explicitly, invalid env values degrade gracefully with a warning, and the new test file covers the key behaviors. No blocking bugs found.

Findings

No actionable findings.

Notes

• The fresh_client fixture correctly pops the GraphQLClient singleton before/after each test so __init__ re-reads the env var — this is the right pattern for testing a singleton that captures env at construction time.
• Considered flagging that _limits is captured once per singleton lifetime and won't reflect runtime env changes, but the docstring on _resolve_keepalive_expiry and the singleton design make this intentional; recorded here rather than as a finding.
• Test coverage is comprehensive for the new behavior (default, override, fallback, ipv4_only transport threading) and follows the repo's "no real sockets, no sleeps" guideline.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.