feat(adk): make AgentEx client HTTP timeouts configurable by env var - #510
Open
chakrris wants to merge 5 commits into
Open
feat(adk): make AgentEx client HTTP timeouts configurable by env var#510chakrris wants to merge 5 commits into
chakrris wants to merge 5 commits into
Conversation
The AgentEx client's timeout comes from the SDK's DEFAULT_TIMEOUT,
Timeout(connect=5.0, read=300, write=300, pool=300). Application code
cannot change it: the ADK constructs its own clients internally at 14
sites (messages, tasks, events, tracing), all through
create_async_agentex_client(), and none accepts a timeout from the caller.
No environment variable controlled it either.
The connect timeout is the one that matters. An AgentEx backend accepts
connections serially, so connect latency grows with the number of
concurrent callers. Measured against a local backend:
concurrency 1 84 ms
concurrency 20 423 ms
concurrency 100 500 ms
concurrency 200 1,010 ms
An agent running 100 concurrent activities, each making ADK calls, pushes
past the 5s budget and fails with httpcore.ConnectTimeout. One run
produced 197 such failures in a single activity. A 5s connect against a
300s read is also internally inconsistent.
Adds AGENTEX_CLIENT_{CONNECT,READ,WRITE,POOL}_TIMEOUT_SECONDS, following
the existing EnvVarKeys and EnvironmentVariables pattern. Defaults equal
the current DEFAULT_TIMEOUT, so an unconfigured process is unchanged. An
explicit timeout= argument still wins, and a malformed value falls back to
the SDK default with a warning rather than preventing client creation.
The EnvironmentVariables symbol is reached through the env_module alias, so the direct import was dead and tripped ruff F401/I001 in CI.
Greptile flagged that adding the four timeout fields to the shared EnvironmentVariables model meant a malformed value broke far more than the client factory: refresh() is called from ~20 unguarded places, including AgentWorker startup and EnvAuth.auth_flow on every request. The try/except in create_async_agentex_client() did not contain that, it only made it look handled. Removing the try/except alone made it worse. agentex/lib/adk/utils/__init__.py constructs TemplatingModule() at module scope, which builds a client, so importing the ADK started requiring AGENT_NAME and ACP_URL to be set. The suppressed exception had been hiding that. Read the four values from os.environ in client.py instead. The shared model is untouched, so startup, auth and import are unaffected, and a malformed value raises a ValueError naming the variable at the point it is used rather than being swallowed. Adds a regression test asserting the timeout does not depend on the shared model.
AsyncAgentex.timeout is typed float | Timeout | None, so pyright rejected reading .connect off it directly. ruff was clean, which is why CI caught this and the local ruff run did not.
Reverting environment_variables.py in the previous commit checked it out from origin/next rather than the merge base, which pulled AGENT_COMMIT_SHA from another commit into this diff. This PR now touches only client.py and its test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The AgentEx client's HTTP timeout is fixed at the SDK default and cannot be changed from application code. This adds four environment variables to configure it, with defaults equal to today's values so nothing changes for an unconfigured process.
The problem
create_async_agentex_client()passes notimeout, so every client falls back toDEFAULT_TIMEOUT:An application cannot override this. The ADK builds its own clients internally at 14 sites across
messages,tasks,eventsandtracing, all through that factory, and none takes a timeout from the caller. No environment variable controlled it either, so wrapping the factory locally only covers the paths an app constructs itself, not the ADK's.The connect timeout is the one that bites. An AgentEx backend accepts connections serially, so connect latency grows with the number of concurrent callers. Measured against a local backend with
GET /readyz, a 10s client timeout, and no failures at any level:Latency is linear in concurrency, which is the signature of serialised accepts rather than slow request handling. An agent running 100 concurrent Temporal activities, each making ADK calls, crosses the 5s budget and fails with
httpcore.ConnectTimeout. One run produced 197 such failures from a single activity type.A 5s connect alongside a 300s read is also internally inconsistent: the client will wait five minutes for a response but only five seconds to open the socket.
What changed
The whole change is in
src/agentex/lib/adk/utils/_modules/client.py._timeout_from_env()reads four variables fromos.environ, andcreate_async_agentex_client()applies the result when the caller passes notimeout:Because all 14 internal construction sites route through that factory, one change covers
messages,tasks,eventsandtracing.Three properties worth checking in review:
DEFAULT_TIMEOUT, so an unconfigured process behaves exactly as before.timeout=argument still wins over the environment.ValueErrornaming the variable, at the point the value is used.Why not
EnvironmentVariablesThe first version of this PR declared the four values as fields on the shared
EnvironmentVariablesmodel, which is the repo's usual pattern. That turned out to be the wrong home for them, and the reasoning is worth recording because the failure was not obvious:EnvironmentVariables.refresh()is called from about 20 places, none of which guard it, includingAgentWorkerstartup andEnvAuth.auth_flow()on every authenticated request. A malformed timeout would therefore fail validation far from the client factory. The factory'stry/exceptdid not contain that; it only made the failure look handled at the one site that could have reported it clearly.Removing that
try/exceptexposed a second problem.agentex/lib/adk/utils/__init__.pyconstructsTemplatingModule()at module scope, and that constructor builds a client, soimport agentex.lib.adkbegan requiringAGENT_NAMEandACP_URLto be set. The suppressed exception had been hiding an import-time dependency on a fully configured environment.Reading
os.environdirectly avoids both. The shared model is untouched, so startup, authentication and import are unaffected, and a bad value affects only the timeout it describes.Review guide
client.pyis the entire behavioural change; it and its test are the only two files this PR touches.tests/lib/test_client_timeout_env.pycovers the defaults, each override, the empty-string case, precedence of an explicit argument, thatEnvAuthsurvives, the malformed-value error, and a regression test asserting the timeouts do not depend on the shared environment model.Testing
uv syncrequiresuv >= 0.9and this machine has 0.8.13, so I ran the suite throughuvxwith both packages installed editable. The 9 new tests pass, andtests/libis 1028 passed / 3 skipped.pyrightat the pinned 1.1.399 is clean on the touched files. The 12 errors in that run are alltests/lib/cli/test_agent_handlers.pyraising on a ClickDeprecationWarningunderfilterwarnings = error; they come from unpinned resolution in the ad-hoc environment rather than fromuv.lock, and are unrelated to this change.ruff checkandruff format --checkare clean on the touched files.Greptile Summary
Adds environment-variable configuration for the AgentEx async client's connect, read, write, and pool timeouts.
Confidence Score: 5/5
The PR appears safe to merge with no outstanding correctness, security, or repository-rule issues.
The previously reported shared-environment validation issue was fully addressed by reading timeout variables directly in the client factory, and that thread is resolved. The current implementation preserves existing defaults and explicit timeout precedence while limiting malformed configuration failures to client creation.
Important Files Changed
Reviews (4): Last reviewed commit: "Drop an unrelated file change from the b..." | Re-trigger Greptile