Skip to content

feat(transport): bounded retry for active services, over an API-neutral layer - #350

Merged
thodson-usgs merged 5 commits into
DOI-USGS:mainfrom
thodson-usgs:refactor/phase-2-transport-boundaries
Aug 6, 2026
Merged

feat(transport): bounded retry for active services, over an API-neutral layer#350
thodson-usgs merged 5 commits into
DOI-USGS:mainfrom
thodson-usgs:refactor/phase-2-transport-boundaries

Conversation

@thodson-usgs

@thodson-usgs thodson-usgs commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Retitled. This started as a boundary extraction and was described as one,
but it also switched WQP, NLDI, StreamStats, and Water Use onto a retrying
request path. That is a user-visible resilience change, not a refactor, so it
now leads the description. Reviewers should evaluate the
retry behavior on its own merits; the layering change is the second half.

What changes for users

Four services now retry transient failures where they previously failed on the
first attempt:
wqp, nldi, streamstats, and wateruse. A request that
hits a 429 or a gateway 5xx is re-sent up to API_USGS_RETRIES times (default 4)
with exponential backoff and full jitter, honoring a server Retry-After.

This costs latency and quota on failing requests, so the retry is bounded on two
independent axes and narrowed to failures a later attempt could actually
survive:

  • API_USGS_STALL_TIMEOUT (new; default 60 s, 0 disables) bounds how long
    a call may go without receiving any data. API_USGS_RETRIES counts
    attempts, not seconds, so on its own four retries of a request that times out
    after a minute is four silent minutes. Progress restarts the budget — a page
    received, or a queued sub-request acquiring its concurrency slot — and an
    attempt already in flight is never interrupted. The first retry is never
    withheld, so one slow attempt cannot disable retry by itself. A dead
    connection costs about two read timeouts (~2 min) instead of five attempts
    (~5 min).
  • Which statuses are re-sent is per-adapter. WQP answers an over-large query
    with a 500 and StreamStats answers out-of-network coordinates with one, so
    those one-shot adapters re-send only for 429/502/503/504. The Water Data OGC
    API is a query interface where a 500 is an upstream hiccup, so the chunker
    keeps re-sending for every 5xx, as it always has.
  • Failures already settled are not retried — an unsupported scheme, a
    malformed request, or a hostname the resolver rejects outright fails on the
    first attempt. A temporary resolver failure (EAI_AGAIN: a resolver still
    coming up, a VPN reconnect, a laptop waking) stays retryable.
  • Backoff always includes jitter, including on a server-named Retry-After,
    so sub-requests handed one hint do not wake in lockstep, and a Retry-After
    of 0 cannot become a zero-delay re-send. An HTTP-date that has already
    passed is treated as no hint rather than as "retry now", since the likelier
    cause is client/server clock skew.

Water Data (waterdata) already retried via the chunker and keeps doing so;
what is new for it is the stall budget, the deterministic-failure
classification, and the jitter above.

Deprecated nwis is untouched — it still uses utils.query, which retains
its exact signature and performs no retry.

A bad setting now raises ConfigurationError (both a DataRetrievalError
and a ValueError), so a typo in API_USGS_RETRIES, API_USGS_STALL_TIMEOUT,
or API_USGS_CONCURRENT no longer escapes a request path as a bare
ValueError.

Measured against the live API

A 4-state, 30-year get_daily over 800 sites at parallel_chunks(1) runs 91.8 s
wall-clock and returns 581,070 rows. Its longest gap between progress events is
12.1 s — the budget measures silence, not duration, so long successful queries
are not at risk. A 2,000-site / 35-year variant: 74.0 s total, 11.2 s worst gap.

The layering change

dataretrieval.transport is a new internal, API-neutral execution layer owning
guarded HTTP client lifecycle and timeout defaults, host-scoped authentication,
cursor pagination, bounded retry, response aggregation, progress, and
sync-over-async dispatch. dataretrieval.ogc keeps its protocol concerns:
dialects, CQL2, request construction, feature shaping, URL-byte chunk planning,
resumable ChunkedCall state, and interruption types.

This is what makes the retry change tractable: before it, generic execution
behavior lived under OGC even where non-OGC services used it, so Water Use
depended on private protocol modules and retry policy was uneven across
services. There is now one retry policy to reason about instead of several.

Internally, transport.liveness is a stdlib-only leaf recording when data last
arrived, so the page loop that observes progress and the retry loop that acts on
it both depend on it rather than on each other.

Not a public framework: dataretrieval.transport is internal and carries no API
promise.

Compatibility

Public imports, service signatures, return shapes, metadata, deprecations,
exception types, OGC chunking/resume behavior, and the exact four-symbol OGC
facade are unchanged. utils.query retains its exact signature and still
performs no retry.

Compatibility aliases are kept where a consumer exists: the private utils
transport names (_get, _default_headers, HTTPX_DEFAULTS, …) and the
ogc.engine wrappers are live re-exports, so existing monkeypatch sites keep
working. Two modules were removed rather than aliased, since nothing in the
tree imported them: dataretrieval.ogc.progress and
dataretrieval.ogc.combining (now dataretrieval.transport.progress /
dataretrieval.transport.combining). dataretrieval.ogc.retry keeps only its
OGC interruption classifiers; the retry tunables it used to re-export were
copies by value that patching could not reach, and now live solely in
dataretrieval.transport.retry.

What changes deliberately is the retry behavior described above.

Commits

Squashed to a single commit, cafe0676, on current main (ec88cec7).

Validation

Re-run on the squashed commit: full suite 667 passed with 57 expected
warnings; strict mypy clean over 41 source files; Ruff lint and format clean;
all configured pre-commit hooks pass.

From the post-rebase validation of 966bb618 (not re-run for the later
commits): coverage 97%, Sphinx HTML build with the same four pre-existing
warnings, isolated wheel build/install/import outside the checkout, and the
GitHub Actions matrix across lint, typing, artifact, docs, Ubuntu, and Windows.

Architecture fitness functions in tests/architecture_test.py enforce transport
dependency direction, an acyclic transport graph, and Water Use isolation from
OGC. ADR 0006 records the decision.

Test-coverage caveat

tests/conftest.py::_pin_chunker_env pins API_USGS_RETRIES=0 for the whole
suite, so the newly default-on retry paths are exercised only where a test opts
back in explicitly. The green suite therefore verifies less of this behavior
change than its size suggests; the live-API measurements above and the targeted
retry tests are the real evidence.

A CI fix rides along

This PR also changes what CI enforces, which is worth a maintainer's attention
because it is not scoped to this branch.

The retry classifier reads the platform's EAI_* constants from socket, but
its test wrote one out literally (8, which is EAI_NONAME on macOS and -2 on
Linux). That failed on Ubuntu — correctly. What it exposed is that the Windows
matrix reported success on the very same failure
: its log reads
1 failed, 665 passed under a green job.

The cause is not continue-on-error. The test step is:

run: |
  coverage run -m pytest tests/
  coverage report -m

run: defaults to PowerShell on Windows, which does not halt on a failing native
command and takes the step's exit status from the last one — so coverage report
returning 0 masked the failed pytest. Ubuntu, on bash -e, stopped at pytest and
never printed a coverage total; Windows printed one. That is the difference in the
logs, and it means every Windows test failure in this repository has been
invisible
, not only this one and not only on this branch.

The fix is one line, shell: bash on that step, so both platforms behave
identically. The Windows jobs in the current run are therefore real signal rather
than a default pass.

Known follow-ups (recorded, not fixed here)

  • .retryable on the exception taxonomy stays the broader "might succeed" hint
    and does not agree with the stricter auto-re-send sets; unifying them changes
    a documented public field and belongs in its own change.
  • ogc.retry._classify_chunk_error still maps some deterministic failures to a
    resumable ServiceInterrupted (pre-existing).
  • Three next-cursor validators exist with three failure modes (wateruse,
    ogc/engine, waterdata/ratings); a shared resolver belongs in transport.
  • The stall default equals the 60 s read timeout, so a request burning a full
    read timeout gets one retry rather than several. Deliberate.

Scope

Collection-family splitting, additional adapter restructuring, and legacy NWIS
retirement remain separate follow-up work.

@thodson-usgs
thodson-usgs force-pushed the refactor/phase-2-transport-boundaries branch from 6a57e29 to 3822f04 Compare August 3, 2026 15:25
@thodson-usgs thodson-usgs changed the title refactor(transport): extract API-neutral execution policy feat(transport): bounded retry for active services, over an API-neutral layer Aug 4, 2026
…al layer

WQP, NLDI, StreamStats, and Water Use now retry transient failures instead
of surfacing the first one. That is a resilience change, not a refactor, so
it leads here; the layering change that made it tractable follows.

Retry costs latency and quota on failing requests, so it is bounded on two
independent axes and narrowed to failures a later attempt could survive.

API_USGS_STALL_TIMEOUT (new; default 60 s, 0 disables) bounds how long a
call may go without receiving any data. API_USGS_RETRIES counts attempts,
not seconds, so on its own four retries of a request that times out after a
minute is four silent minutes. Progress restarts the budget -- a page
received, or a queued sub-request acquiring its concurrency slot, credited
as the wait it was rather than restamped -- and an attempt already in
flight is never interrupted. The first retry is never withheld, so one slow
attempt cannot disable retry by itself. A dead connection costs about two
read timeouts rather than five attempts.

Which statuses are re-sent is per-adapter. WQP answers an over-large query
with a 500 and StreamStats answers out-of-network coordinates with one, so
those one-shot adapters re-send only for 429/502/503/504. The Water Data
OGC API is a query interface where a 500 is an upstream fault, so the
chunker keeps re-sending for every 5xx, as it always has.

Failures already settled are not retried: an unsupported scheme, a request
we built wrong, or a hostname the resolver rejects outright. A temporary
resolver failure (EAI_AGAIN) stays retryable. Backoff always includes
jitter, including on a server-named Retry-After, so sub-requests handed one
hint do not wake in lockstep and a hint of 0 cannot become a zero-delay
re-send. An unusable setting raises ConfigurationError -- both a
DataRetrievalError and a ValueError -- rather than escaping a request path
untyped.

Measured against the live API: a 4-state, 30-year get_daily over 800 sites
at parallel_chunks(1) runs 91.8 s and returns 581,070 rows with a worst
inter-page silence of 12.1 s, so the budget does not threaten long
successful queries.

The layering half adds dataretrieval.transport, an internal API-neutral
execution layer owning guarded client lifecycle and timeout defaults,
host-scoped authentication, cursor pagination, bounded retry, response
aggregation, progress, and sync-over-async dispatch. dataretrieval.ogc
keeps its protocol concerns: dialects, CQL2, request construction, feature
shaping, URL-byte chunk planning, resumable ChunkedCall state, and
interruption types. Before this, generic execution behavior lived under OGC
even where non-OGC services used it, so Water Use depended on private
protocol modules and retry policy was uneven across services; there is now
one policy to reason about. transport.liveness is a stdlib-only leaf
recording when data last arrived, so the page loop that observes progress
and the retry loop that acts on it depend on it rather than on each other.
Architecture fitness functions enforce the dependency direction, an acyclic
transport graph, and Water Use's isolation from OGC; ADR 0006 records the
decision.

Compatibility: public imports, service signatures, return shapes, metadata,
deprecations, exception types, OGC chunking/resume behavior, and the
four-symbol OGC facade are unchanged, and utils.query keeps its exact
signature and performs no retry. Private compatibility aliases are kept
where a consumer exists. Two modules were removed rather than aliased,
since nothing imported them: ogc.progress and ogc.combining, now
transport.progress and transport.combining. ogc.retry keeps only its OGC
interruption classifiers.

Also pins the CI test step to bash on every OS. Windows defaults to
PowerShell, which does not halt on a failing native command and takes the
step's exit status from the last one, so a coverage report following a
failed pytest reported success -- every Windows test failure in this
repository has been invisible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015UVSiGgUyDRSbqNWM5HBbL
thodson-usgs and others added 2 commits August 5, 2026 20:13
Splits three concerns out of the shared transport layer and closes two
credential-leak paths in server-supplied pagination links.

The layer was named "API-neutral" but held `api.waterdata.usgs.gov`, read
`API_USGS_PAT`, pinned `x-ratelimit-remaining`, and printed a USGS signup
URL. It is neutral across USGS *services*, not across HTTP APIs, and the
aspirational name invited generality nobody needs. ADR 0006 now says so
plainly and is renamed to match.

Two modules were in transport only because they had to leave `ogc/`
during the earlier extraction:

- `progress.py` is terminal presentation (Jupyter detection, status-line
  rewriting, broken-pipe handling), called *from* transport rather than
  part of it, and the sole reason a `progress -> http` edge existed.
- `combining.py` is DataFrame assembly, consumed by `ogc/planning` and
  `wateruse` for reasons unrelated to HTTP.

Both move to top-level leaves. Transport goes 1290 -> 766 lines and 7 ->
5 modules, and `http`/`liveness` become leaves.

A new `credentials.py` leaf owns every answer about the API key. The code
that attaches a credential and the code that strips it back off have to
agree on which host is authorized, and the way they stop agreeing is a
second copy of the host string. `waterdata/utils`, `ogc/policy`, and
`ngwmn` each carried their own `BASE_URL` spelling of that same
authority -- two of them with a comment documenting the duplication as
deliberate -- so they now derive it from the one definition.

Closes two ways a poisoned response body reached a credential:

- `accepts_api_key` matched on host alone, so `http://` on the
  authorized host sent the key in cleartext. It now requires https.
- `ogc/engine` checked the next-link host but not its userinfo, and
  `waterdata/ratings` followed STAC `next` hrefs with no check at all.
  httpx derives `Authorization: Basic` from userinfo, so a link carrying
  `user:pass@` minted a credential the caller never configured and sent
  it beside the real API key -- past the host check, which passes in
  exactly that case.

The credential fitness function matched the quoted bare host, so the
`https://`-prefixed form slipped past it and it reported success with
three copies live. It now walks AST string values, excluding docstrings
so prose naming the service is not mistaken for a second source of truth.

Every new test was verified to fail against the unfixed source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Should we retry this?" and "can the caller resume it?" are the same
question asked twice, and the two answers disagreed. Retry already
declines to re-send a failure no later attempt could survive -- a bad
URL scheme, a malformed request, a hostname the resolver rejects
outright. The interruption classifier mapped every httpx error to
ServiceInterrupted regardless, so the caller got a .call.resume() whose
every attempt fails identically, with the NetworkError that actually
explained the problem buried underneath it.

Both answers now come from one predicate in transport. A deterministic
failure classifies as unrecognized, which is the existing "re-raise raw"
path, so the caller sees the real error.

The test asserts both answers on the same failures, so they cannot drift
apart again. Note the chain shape matters: our wrapper raises with
`from`, so the chunker's explicit-link walk reaches the httpx error,
whose implicit links then lead to the resolver code -- a temporary
resolver failure stays both retryable and resumable, decided only by the
errno.

Addresses finding 3 of the chunking review; finding 4 (a 5xx sibling
masking a 429's Retry-After) remains open.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread NEWS.md Outdated
@thodson-usgs
thodson-usgs marked this pull request as ready for review August 6, 2026 18:39
@thodson-usgs
thodson-usgs merged commit 31b0142 into DOI-USGS:main Aug 6, 2026
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