feat(http): add a client timeout default and a scoped per-request override - #48
Conversation
…rride HttpClient/SyncHttpClient hardcoded a 30s httpx timeout with no way to change it, so callers running under an overall budget could not bound SDK calls. Add a `timeout=` constructor keyword (default unchanged) and `request_timeout(seconds)`, a ContextVar-scoped override honored by every request, raw, and SSE stream path, so generated resource methods honor it without signature changes. A non-positive budget raises TimeoutError before sending. Refs ArchAstro/firstlanding#13731 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013CN15q1LWb444eMZSkQi9M
|
Orchestrator review (read-only, verified on This is the decision #13731 asked for, made the right way: a runtime-level default plus a context-scoped per-request override, no generator change, and generated resource methods proven to honor it end to end ( Non-blocking:
Release as 0.8.0 per the description; the firstlanding PR that threads |
|
Rob's call: fold the three non-blocking notes above into this PR before release rather than deferring them.
Add the re-export to the existing generated-resource test (import from |
… timeouts Re-export `request_timeout` and `DEFAULT_TIMEOUT_S` from `archastro.platform` and teach that import in the README. Document that a 401 refresh request and its retry each get the override, and pin it with sync and async tests that send the refresh through a separate refresh-only client, as the generated `with_credentials` builder does. Refs ArchAstro/firstlanding#13731 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013CN15q1LWb444eMZSkQi9M
|
Folded in all three notes in b3fa743.
Local: |
Problem and author intent
HttpClientandSyncHttpClientbuild their httpx client with a hardcodedtimeout=30.0. No constructor, request, or stream path takes a timeout, so a caller cannot bound an SDK call.This matters for the ArchAstro benchmark harness (ArchAstro/firstlanding#13731). Its provider contract passes each step a budget (
timeout_s) that the provider turns into a deadline, giving every platform call the time that remains. The raw-httpx providers do this. The SDK-backed arm cannot, soRunOptions.ingest_timeoutandsearch_timeouthave no effect on the arm whose numbers get published. A slow platform fails the two arms in different ways: 30s per request with no overall bound on one side, a bounded step on the other.The goal is a runtime-level timeout that generated resource methods honor without changing their signatures.
What changed
Decision the issue asked for: a client default plus a context-scoped per-request override, both in the hand-maintained runtime. No per-method
timeout=on generated resources. That would be a generator change, and the harness does not need it: it wraps calls it already makes.HttpClient(..., timeout=30.0)/SyncHttpClient(..., timeout=30.0): a keyword with the old value as default. Existing callers see no change.request_timeout(seconds), exported fromarchastro.platform(defined inarchastro.platform.runtime.http_client, alongsideDEFAULT_TIMEOUT_S): a context manager backed by aContextVar. Everyrequest,request_raw,stream_sse, andstream_sse_syncsent inside the block uses that httpx timeout instead of the client default. Generated resource methods go through these paths, soclient.knowledge_documents.list(...)is covered with no generator change.TimeoutErrorbefore anything is sent. This matches the harness's raw client (timeout_s <= 0short-circuit), so a caller passing down an exhausted deadline fails fast rather than issuing a request with a nonsensical timeout.Before → after for a harness step with 12s left:
Semantics worth knowing:
with_credentialswires it); that client reads the same context, so it cannot fall back to its 30s default. Callers that need one deadline across a chain pass each call the remainder, which is what the harness does.asyncio.to_thread). A plainthreading.Threaddoes not inherit it, so parallel harness workers cannot leak budgets into each other.Intentionally unchanged:
httpx.TimeoutExceptionis not translated toTimeoutError. Changing the exception type existing callers see is a separate decision; the benchmark provider translates at its own boundary.src/archastro/platform/__init__.pyis generator output, and the re-export is hand-added to it. The@archastro/sdk-generatorpackage-init emitter does not know about it yet, so a regeneration would drop the line.test_generated_resource_methods_honor_request_timeoutimports from the package root, so a regen that drops it fails CI instead of silently removing public API. The generator follow-up is listed below.PlatformClient/AsyncPlatformClientbuilders (client.pyis generator output) do not passtimeout=through yet. The context override covers every client they build. A builder passthrough can go into the generator when a consumer needs a different default.Testing
New tests in
tests/test_http_client.pyread the timeout httpx actually attaches to the outgoing request (request.extensions["timeout"]throughhttpx.MockTransport), not the arguments handed to httpx:timeout=sets the client default (sync and async)requestandrequest_raw(sync and async)TimeoutErrorwith zero requests recorded, acrossrequest,request_raw, and the stream path (sync and async)PlatformClient.with_secret_key(...).knowledge_documents.list) honors it end to end through the real generated client, withrequest_timeoutandDEFAULT_TIMEOUT_Simported fromarchastro.platform, the public pathThree existing tests asserted the exact
request(...)kwargs and now includetimeout=httpx.USE_CLIENT_DEFAULT.Mutation checks, run locally: removing the non-positive guard fails the four expired-budget tests; dropping
timeout=from the stream calls fails both stream tests; making the refresh-only client ignore the override fails both refresh tests.uv run pytest tests/test_http_client.py src/archastro/phx_channel/tests/test_unit.py tests/examples: 111 passed (64 intest_http_client.py)uv run pytest tests/harness tests/contract: 2694 passeduv run ruff check,uv run ruff format --check: cleanEnd-to-end proof: none added here, per #13731 ("Do not add a new E2E"). This is a runtime primitive with no process boundary beyond httpx. Its consumer-side proof is the benchmark provider PR that follows. That PR covers the SDK-arm path in
//benchmarks:test-ci, and the existingservices/elixir/evals/test/remote_eval/python_harness_capability_e2e_test.exscovers the arm on a real platform.Scope, risk, user impact
SDK runtime only. Risk: low. The public API gains one keyword and one context manager. With neither used, the only change is that each httpx call now receives an explicit
timeout=USE_CLIENT_DEFAULT, which httpx treats as the default it already applied. No user-facing behavior change unless the new knobs are used.Release and follow-ups
release.yml(minor, since this adds public API; 0.7.1 → 0.8.0), then the firstlanding PR bumps both benchmark pins and threadstimeout_sthroughArchastroDocStoreSDK.@archastro/sdk-generatorPython package-init emitter (generatePackageInitinarchastro-openapi) to re-exportrequest_timeoutandDEFAULT_TIMEOUT_S, so regeneration keeps them. This repo locks the generator at^0.9.0against a published 0.11.7, so it lands with the next generator bump.timeout=on generated resources and atimeout=passthrough on the generated builders. Both wait for a consumer that needs them.🤖 Generated with Claude Code
https://claude.ai/code/session_013CN15q1LWb444eMZSkQi9M