Skip to content

refactor(async): move retries and client ownership onto a shared transport - #323

Merged
Mattsface merged 1 commit into
feature/305-async-lookupsfrom
refactor/async-shared-transport
Aug 23, 2026
Merged

refactor(async): move retries and client ownership onto a shared transport#323
Mattsface merged 1 commit into
feature/305-async-lookupsfrom
refactor/async-shared-transport

Conversation

@Mattsface

Copy link
Copy Markdown
Member

Why

The async side's ownership pointed the wrong way relative to the sync design.

AsyncMlb.__init__ built the v1 adapter with client=client, let that
adapter resolve and create the shared httpx.AsyncClient, then reached back
in for self._mlb_adapter_v1._client to construct the v1.1 adapter.
AsyncMlb.aclose() was await self._mlb_adapter_v1.aclose() — the parent's
shutdown delegated to one of its children, with the other child required not
to close the thing it shares.

Underneath that, AsyncMlbDataAdapter._owns_client answered two different
questions at once: "who closes this client" and "am I allowed to retry." The
sync side keeps those separate — Mlb._owns_session answers only the first,
and retries are mounted onto the Session once by _configure_library_session().
Because async fused them, the v1.1 adapter (which always receives a
non-None client, since it borrows v1's) concluded it was using a
caller-injected client and disabled retries even when the client was
library-owned. AsyncMlbDataAdapter._set_retries_enabled() existed purely to
undo that wrong conclusion after construction — a real fix for a real bug,
landed two commits ago on feature/305-async-lookups, but the wrong layer for
it.

HTTPX has the same extension seam requests has: AsyncClient(transport=...)
accepts any AsyncBaseTransport, the position HTTPAdapter occupies in
Requests. Moving the retry loop there instead of leaving it inside
AsyncMlbDataAdapter makes retries a property of the client, not of either
adapter, so two adapters sharing one client share one policy by construction
and structurally cannot disagree about it. No flag needed.

What changed

New private module mlbstatsapi/_async_transport.py:

  • MlbAsyncRetryTransport(httpx.AsyncBaseTransport) — the retry loop
    currently in AsyncMlbDataAdapter._request_with_retries /
    _sleep_before_retry, moved here with identical budget accounting and
    backoff timing (see table below). On an exhausted budget it re-raises the
    underlying HTTPX exception rather than translating it — transports are
    contractually expected to raise HTTPX errors; translation to
    MlbTimeoutError/MlbTransportError stays with the adapter.
  • create_library_async_client() — the async counterpart of
    _configure_library_session(): builds a client with the package
    User-Agent and MlbAsyncRetryTransport() mounted.

AsyncMlb.__init__ now owns the client exactly as Mlb.__init__ owns the
Session: self._owns_client, self._client, self._closed, created via
create_library_async_client() when the caller passes none, handed to
both adapters. aclose() closes self._client directly instead of
delegating to the v1 adapter.

AsyncMlbDataAdapter's public constructor is unchanged. get() now calls
self._client.get() directly, wrapped in the same two except clauses the
sync adapter's error path mirrors:

except httpx.TimeoutException as exc:
    raise MlbTimeoutError("Request failed") from exc
except httpx.RequestError as exc:
    raise MlbTransportError("Request failed") from exc

Deleted _set_retries_enabled, _retries_enabled, _retry_policy,
_request_with_retries, _sleep_before_retry.

Retry budget/backoff accounting, preserved exactly:

failure budget
httpx.ReadTimeout policy.read
httpx.ConnectTimeout policy.connect
httpx.ConnectError policy.connect
other httpx.TimeoutException policy.total
other httpx.RequestError policy.total
status in policy.status_forcelist policy.status

Target shape, matching Mlb/Session exactly:

AsyncMlb
├── owns/creates the AsyncClient (retry transport mounted at creation)
├── v1 adapter   — borrows the client
└── v1.1 adapter — borrows the client

docs/public-api.md — the "API versions used by AsyncMlb" section now
says AsyncMlb owns the shared client and that retries are a property of
that client's transport, not of either adapter. Checked
docs/http-transport.md: it's explicitly scoped to version 1.0.0
("Async support is not part of version 1.0.0") and its "v1.1 adapter"
reference is about Mlb's sync adapter — out of scope, no changes needed.

Tests:

  • Retargeted the sleep patch and the transport-construction patch seam from
    mlbstatsapi.async_mlb_dataadapter.httpx.AsyncClient to
    mlbstatsapi._async_transport.httpx.AsyncHTTPTransport, so tests exercise
    the real client, the real retry transport, and the real headers with a
    MockTransport at the bottom. _owned_adapter/_injected_adapter keep
    their names; added _retry_policy_of() since the policy now lives on the
    client's transport.
  • Replaced the two _set_retries_enabled tests with three: a
    library-created client mounts MlbAsyncRetryTransport; an injected
    client's transport is left exactly as supplied; mounting
    MlbAsyncRetryTransport on an injected client makes it retry (the
    caller-facing opt-in, mirroring the documented sync create_retry_policy()
    recipe).
  • In tests/test_async_mlb.py, ownership assertions moved from the adapters
    to AsyncMlb (mlb._client, mlb._owns_client, both adapters'
    ._client is mlb._client, neither adapter owns it). The cleanup-failure
    test now mocks mlb.aclose rather than an adapter's.
  • Also fixed a stale docstring in an unrelated nearby test
    (test_retry_sleep_is_async_and_non_blocking) that still named
    _sleep_before_retry, which no longer exists.

How it was tested

poetry run pytest tests/ --ignore=tests/external_tests
  • Before (branch tip prior to this change): 973 passed
  • After: 974 passed (net +1: 2 removed, 3 added)

tests/test_sync_async_parity.py: 96 passedAsyncMlb's observable
behavior against Mlb did not move.

poetry run pytest tests/external_tests/async_mlb/ against the live API:
26 passed.

Also verified directly (not just via assertions) that: both adapters' clients
are identical objects and neither adapter itself owns the shared client; a
caller-injected client's transport is left untouched and the client is never
closed by the library; aclose() on a library-owned client actually closes
it and is idempotent.

Risk level

Low-to-moderate. This is a structural move of existing, already-tested retry
logic into a different object (a httpx.AsyncBaseTransport instead of a
loop inside the adapter), not new behavior. The public constructors of
AsyncMlb and AsyncMlbDataAdapter are unchanged. The main risk is a subtle
behavioral drift in the retry/backoff/exception-mapping path, which is why
the budget/backoff/exception-mapping table above is preserved verbatim and
covered by the same test assertions as before (relocated, not weakened).

Possible impact

None for public API consumers — AsyncMlb(...) and AsyncMlbDataAdapter(...)
constructor signatures, aclose()/context-manager semantics, retry behavior,
and exception types are all unchanged from the outside. Internal/private
attribute reachers (none exist outside this repo's own test suite, since
_mlb_adapter_v1, _client, _retry_policy, etc. are all explicitly
documented as private) would need to update their attribute paths, but no
such external reachers should exist since these are unmistakably private
(leading underscore) and not covered by the stability policy.

What was intentionally left out — decision needed

Should MlbAsyncRetryTransport be exported publicly?

Right now it's implemented as private (mlbstatsapi/_async_transport.py,
not re-exported from the package root). A caller who wants library retry
behavior on a client they own and inject can already do this themselves —
it's demonstrated in the new
test_mounting_the_retry_transport_makes_an_injected_client_retry test — but
only by reaching into a private module.

Making it public (from mlbstatsapi import MlbAsyncRetryTransport, alongside
the existing create_retry_policy()) would turn "an injected client gets no
library retries" from a documented limitation into a documented, supported
opt-in — the async analog of the sync create_retry_policy() recipe already
in docs/http-transport.md. That's a public-API surface decision, not an
implementation detail, so I did not make it unilaterally. Left private for
now; happy to add the export, the package-root manifest entry, and a
docs/public-api.md/docs/http-transport.md recipe section if that's the
direction you want.

I did not change whether an injected client retries by default — it still
does not, and with this design that falls out structurally (an injected
client's transport is whatever the caller gave it) rather than from a flag.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto

…sport

AsyncMlb's ownership pointed the wrong way: __init__ built the v1 adapter
with client=client, let that adapter create the shared httpx.AsyncClient, then
reached back into self._mlb_adapter_v1._client to construct the v1.1 adapter.
aclose() delegated the parent's shutdown to one of its children, with the
other child required not to close the thing it shares. Underneath that,
AsyncMlbDataAdapter._owns_client answered two different questions at once
("who closes this client" and "am I allowed to retry"), which is why
_set_retries_enabled() existed at all: to undo the wrong conclusion the v1.1
adapter reached about retry eligibility after receiving a non-None client.

Mlb does not have this problem because it does not implement retries -- it
configures them, once, by mounting an HTTPAdapter carrying the retry policy
onto the Session it creates. HTTPX has the same extension seam:
AsyncClient(transport=...) accepts any AsyncBaseTransport, the position
HTTPAdapter occupies in Requests. Moving the retry loop there makes retries a
property of the client, so two adapters sharing one client share one policy
by construction and cannot disagree about it.

Adds a private mlbstatsapi/_async_transport.py:
- MlbAsyncRetryTransport wraps an inner transport (default
  httpx.AsyncHTTPTransport()) with the existing create_retry_policy() budget
  and backoff accounting, moved verbatim from
  AsyncMlbDataAdapter._request_with_retries /_sleep_before_retry. On an
  exhausted budget it re-raises the underlying httpx exception rather than
  translating it -- transports are contractually expected to raise httpx
  errors, translation stays with the adapter.
- create_library_async_client() is the async counterpart of
  _configure_library_session(): builds a client with the package User-Agent
  and MlbAsyncRetryTransport() mounted.

AsyncMlb.__init__ now owns the client exactly as Mlb.__init__ owns the
Session (self._owns_client, self._client, self._closed, created via
create_library_async_client() when the caller passes none) and hands that one
client to both adapters. aclose() closes self._client directly instead of
delegating to the v1 adapter.

AsyncMlbDataAdapter's public constructor is unchanged. get() now calls
self._client.get() directly, wrapped in the same two except clauses the sync
adapter's error path mirrors (TimeoutException -> MlbTimeoutError,
RequestError -> MlbTransportError; ConnectTimeout/ConnectError fall into the
right one because ConnectTimeout is a TimeoutException subclass). Deleted
_set_retries_enabled, _retries_enabled, _retry_policy,
_request_with_retries, and _sleep_before_retry.

Retargets the test seam from patching httpx.AsyncClient to patching
httpx.AsyncHTTPTransport inside _async_transport, so tests exercise the real
client, the real retry transport, and the real headers with a MockTransport
at the bottom. _owned_adapter/_injected_adapter keep their names; a new
_retry_policy_of() accessor reads the policy from the client's transport for
tests that mutate it. Replaces the two _set_retries_enabled tests with three
that assert the new shape: a library-created client mounts
MlbAsyncRetryTransport, an injected client's transport is left exactly as
supplied, and mounting MlbAsyncRetryTransport on an injected client makes it
retry (the caller-facing opt-in). In test_async_mlb.py, ownership assertions
move from the adapters to AsyncMlb itself, and the cleanup-failure test mocks
mlb.aclose rather than an adapter's.

Testing:
- poetry run pytest tests/ --ignore=tests/external_tests: 973 passed before
  this change (checked out from the unmodified branch tip), 974 after (net
  +1: two _set_retries_enabled tests removed, three new transport-ownership
  tests added).
- tests/test_sync_async_parity.py: 96 passed, unchanged -- AsyncMlb's
  observable behavior did not move.
- tests/external_tests/async_mlb/: 26 passed against the live API.

Risk: internal-only change to a private transport layer behind AsyncMlb's and
AsyncMlbDataAdapter's unchanged public constructors. Retry budgets, backoff
timing, exception mapping, 404/strict_http behavior, the User-Agent, and
caller-injected-client ownership rules are all covered by tests and were not
changed on purpose.

Intentionally left out: MlbAsyncRetryTransport stays private. Exporting it
would turn "an injected client gets no library retries" from a limitation
into a documented opt-in, which is a public-API decision -- see the PR
description.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
@Mattsface
Mattsface merged commit 81672b0 into feature/305-async-lookups Aug 23, 2026
1 check failed
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