fix(helpers): give the Atoms REST helpers a request timeout - #114
CaptainAni187 wants to merge 1 commit into
Conversation
requests applies no timeout of its own, so the 28 calls in Audience, CallAnalytics, Campaign and KB wait forever on a half-open connection. AgentTools, the fifth helper in the same package, already threads a request_timeout through every call, so take the same keyword-only param and default there.
|
crim doesn't review pull requests automatically here. Comment |
|
crim review |
|
crim is reviewing this pull request. Findings will be posted shortly. |
There was a problem hiding this comment.
LGTM
One-sentence assessment: A correct, backward-compatible fix that closes real hang-forever risk by giving every Atoms REST helper call a request timeout.
What this PR does
Adds a keyword-only request_timeout parameter (default DEFAULT_REQUEST_TIMEOUT = 30.0) to the Audience, CallAnalytics, Campaign, and KB constructors and passes timeout=self.request_timeout to every requests call in those helpers. This matches the pattern already used by AgentTools, whose 30.0 default the new tests assert against. A parametrized test suite verifies every helper request carries a timeout, honors a caller-supplied value, and that the default matches AgentTools.
Findings
No issues found.
- The new parameter is keyword-only with a default, so existing positional callers (
base_url,api_key) are unaffected. requeststimeout is per-connect/per-read, not a total deadline, so the 30s default does not prematurely abort large multipart uploads inkb.upload_document/audience.createthat keep making progress.- Default value is kept in sync with
AgentToolsvia an explicit test.
Good to mergeEvery request finally wears a watch instead of waiting forever on a half-open socket, and there's a test that fails if anyone forgets to strap one on. Genuinely tidy work. |
requestsapplies no timeout unless you pass one, so a call withouttimeout=blocks until the OS gives up on the socket, which on a half-open connection can be many minutes. The Atoms REST helpers make 31 calls and 28 of them had none:AgentTools, the fifth helper in the same package, already gets this right. It takes a keyword-onlyrequest_timeout: float = 30.0, stores it, and passes it to every call:So this is
Audience,CallAnalytics,CampaignandKBcatching up with the one helper beside them that already does it.It matters most where these helpers are actually used. A voice agent reading call analytics or a knowledge base inside a request path has no way to bound the wait today, and the generated client next to it enforces 60s by default, so the hand-written helpers are the one place where a hang can happen.
The change
The same keyword-only
request_timeoutparam, the same30.0default, stored on the instance and passed to all 28 call sites. Nothing else moves: no new module, no shared constant to import, no change to any existing argument, and the param is keyword-only so no positional call breaks.versioning.pyand_envelope.pymake no HTTP calls, so they are untouched.Tests
tests/custom/test_helpers_request_timeout.pywalks every public method on each of the four managers withrequestsreplaced by a recorder, then asserts that no recorded call went out without a timeout. Driving it by reflection rather than naming 28 methods means a helper method added later is covered the day it lands.Twelve cases: every call carries a timeout, a caller-supplied value reaches all of them, and the default equals the one
AgentToolsalready uses so the two cannot drift. All twelve fail onmain.src/smallestai/atoms/helpers/**is.fernignored, so a regen keeps this.Worth saying:
30.0matchesAgentTools, not the generated client's60. I took the sibling helper as the closer precedent, but happy to move all five to 60 if you would rather they match the client.