Skip to content

feat(http): add a client timeout default and a scoped per-request override - #48

Merged
rob-archastro merged 2 commits into
mainfrom
feat/runtime-timeout-override
Sep 15, 2026
Merged

rob-archastro merged 2 commits into
mainfrom
feat/runtime-timeout-override

Conversation

@rob-archastro

@rob-archastro rob-archastro commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Problem and author intent

HttpClient and SyncHttpClient build their httpx client with a hardcoded timeout=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, so RunOptions.ingest_timeout and search_timeout have 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 from archastro.platform (defined in archastro.platform.runtime.http_client, alongside DEFAULT_TIMEOUT_S): a context manager backed by a ContextVar. Every request, request_raw, stream_sse, and stream_sse_sync sent inside the block uses that httpx timeout instead of the client default. Generated resource methods go through these paths, so client.knowledge_documents.list(...) is covered with no generator change.
  • A value that is not positive, NaN included, raises builtin TimeoutError before anything is sent. This matches the harness's raw client (timeout_s <= 0 short-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:

before  client.agents.search(...)                               -> httpx timeout 30s (fixed)
after   with request_timeout(12.0): client.agents.search(...)   -> httpx timeout 12s
after   with request_timeout(0.0):  client.agents.search(...)   -> TimeoutError, no request

Semantics worth knowing:

  • Per request, not a total for the block. After a 401, the token refresh request and the retry each get the same value again, so one call can spend up to three times it. The refresh goes through a separate refresh-only client (as with_credentials wires 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.
  • Scope is the current context: one thread, or one asyncio task and anything that copies its context (asyncio.to_thread). A plain threading.Thread does not inherit it, so parallel harness workers cannot leak budgets into each other.
  • Streams read the value when iteration starts, not when the generator is created. The docstring and README say so.
  • Blocks nest; the innermost wins and the outer value comes back on exit.

Intentionally unchanged:

  • httpx.TimeoutException is not translated to TimeoutError. Changing the exception type existing callers see is a separate decision; the benchmark provider translates at its own boundary.
  • src/archastro/platform/__init__.py is generator output, and the re-export is hand-added to it. The @archastro/sdk-generator package-init emitter does not know about it yet, so a regeneration would drop the line. test_generated_resource_methods_honor_request_timeout imports 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.
  • Generated PlatformClient / AsyncPlatformClient builders (client.py is generator output) do not pass timeout= 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.py read the timeout httpx actually attaches to the outgoing request (request.extensions["timeout"] through httpx.MockTransport), not the arguments handed to httpx:

  • default is still 30s; the constructor timeout= sets the client default (sync and async)
  • override applies inside the block and reverts after it, for request and request_raw (sync and async)
  • nested blocks: innermost wins, outer restored
  • a 401 carries the override onto the refresh request (sent by a separate refresh-only client) and the retry, sync and async; the async case depends on the refresh task copying the caller's context
  • an override held in another thread does not affect this thread's request
  • a non-positive or NaN budget raises TimeoutError with zero requests recorded, across request, request_raw, and the stream path (sync and async)
  • both SSE stream paths honor the override
  • a generated resource method (PlatformClient.with_secret_key(...).knowledge_documents.list) honors it end to end through the real generated client, with request_timeout and DEFAULT_TIMEOUT_S imported from archastro.platform, the public path

Three existing tests asserted the exact request(...) kwargs and now include timeout=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 in test_http_client.py)
  • uv run pytest tests/harness tests/contract: 2694 passed
  • uv run ruff check, uv run ruff format --check: clean

End-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 existing services/elixir/evals/test/remote_eval/python_harness_capability_e2e_test.exs covers 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

  • After merge: release through release.yml (minor, since this adds public API; 0.7.1 → 0.8.0), then the firstlanding PR bumps both benchmark pins and threads timeout_s through ArchastroDocStoreSDK.
  • Not filed: teach the @archastro/sdk-generator Python package-init emitter (generatePackageInit in archastro-openapi) to re-export request_timeout and DEFAULT_TIMEOUT_S, so regeneration keeps them. This repo locks the generator at ^0.9.0 against a published 0.11.7, so it lands with the next generator bump.
  • Not filed: per-method timeout= on generated resources and a timeout= passthrough on the generated builders. Both wait for a consumer that needs them.

🤖 Generated with Claude Code

https://claude.ai/code/session_013CN15q1LWb444eMZSkQi9M

…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
@rob-archastro

Copy link
Copy Markdown
Contributor Author

Orchestrator review (read-only, verified on 69737c0, all three Python legs green): GO.

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 (PlatformClient.with_secret_key(...).knowledge_documents.list inside the block). The tests read request.extensions["timeout"], which is the only level that proves httpx actually received the value. The expired-budget guard runs before _get_token(), so an exhausted deadline sends nothing, not even a refresh; the thread-isolation test covers the harness's worker-thread shape, and because the provider will set/reset per call inside a with block, ThreadPoolExecutor's per-worker context persistence is safe here.

Non-blocking:

  • The 401 refresh request itself is issued outside _do_fetch, so it runs on the client default, not the override. A caller with 2s of budget left can spend up to 30s inside a refresh. Fine for the harness (long-lived secret key, no refresh), but worth one sentence in the docstring alongside the "per request, not per block" note.
  • request_timeout lives at archastro.platform.runtime.http_client. Since it is now public API and the README teaches it, re-export it from archastro.platform so consumers do not import from a runtime-internal path.
  • Builtin TimeoutError for the pre-send check versus httpx.TimeoutException for a real wire timeout is a reasonable split and the PR says so; the benchmark provider's failure classifier already treats both as retryable timeout.

Release as 0.8.0 per the description; the firstlanding PR that threads timeout_s through ArchastroDocStoreSDK must re-derive the ingest budget from the 2026-09-15 evidence before wiring it (see the note on #13731).

@rob-archastro

Copy link
Copy Markdown
Contributor Author

Rob's call: fold the three non-blocking notes above into this PR before release rather than deferring them.

  1. Re-export request_timeout (and DEFAULT_TIMEOUT_S) from archastro.platform and switch the README example to that import.
  2. Docstring and README: one sentence stating that a 401 refresh request runs on the client default, not the override.
  3. Optional, only if cheap: route the refresh request through the same timeout resolution so an exhausted budget cannot spend up to 30s in a refresh. If that touches generated code or the auth path in a non-trivial way, skip it and keep the docstring note.

Add the re-export to the existing generated-resource test (import from archastro.platform) so the public path is what is tested. Same PR, same review.

… 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
@rob-archastro

Copy link
Copy Markdown
Contributor Author

Folded in all three notes in b3fa743.

  1. Re-export. request_timeout and DEFAULT_TIMEOUT_S are exported from archastro.platform, and the README example uses that import. test_generated_resource_methods_honor_request_timeout now imports from the package root. Caveat: platform/__init__.py is generator output, so the line is hand-added. A regen would drop it, but that test would then fail CI instead of the export quietly disappearing. The proper fix is in the generator's generatePackageInit (archastro-openapi). It's listed as a follow-up for the next generator bump, since this repo still locks ^0.9.0.
  2. Refresh semantics. On inspection, the refresh request is not sent on the client default. with_credentials sends it through a separate refresh-only client's request → _do_fetch, and that path reads the same ContextVar. The sync handler runs on the caller's thread; the async refresh runs in a task that copies the caller's context. The docstring and README now say what actually happens: the refresh request and the retry each get the override, so one call can spend up to three times it.
  3. Pinned with tests instead of new routing, since the routing already works: test_sync_refresh_request_and_retry_use_the_same_override and the async version send the refresh through a real refresh-only client and assert the timeout on all three requests. If the refresh-only client ignores the override, both tests fail.

Local: test_http_client.py + unit + examples 111 passed; harness + contract 2694 passed; ruff clean.

@rob-archastro
rob-archastro merged commit c7ef03a into main Sep 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant