Retry transient ontology service failures with exponential backoff

[cmungall]

Summary

Adds automatic retry logic for transient ontology service failures (timeouts, 5xx errors, 429s) so that a single stalled request no longer fails an entire validation run. Implements a configurable RetryPolicy with exponential backoff that is applied to all network-touching operations in OntologyAccess and DynamicEnumPlugin.

Key Changes

• New retry infrastructure (oak_utils.py):

  • RetryPolicy dataclass: configurable retry count and exponential backoff
  • retry_on_service_unavailable(): decorator-like function that retries only OntologyServiceUnavailableError, not definitive failures
  • parse_retry_config() and resolve_retry_policy(): configuration parsing with precedence (CLI flag > oak_config.yaml > defaults)
  • DEFAULT_SERVICE_RETRIES = 2 and DEFAULT_SERVICE_RETRY_BACKOFF = 1.0

• OntologyAccess integration:

  • Constructor accepts service_retries and service_retry_backoff parameters
  • New retry_service_call() method funnels all network operations through retry logic
  • Wraps get_label(), is_obsolete(), entity_aliases(), and adapter construction with retries
  • Adapter construction failures are now classified as service unavailability (retried) vs. configuration errors (fail-fast)

• CLI enhancements (cli.py):

  • New --retries and --retry-wait flags on validate-schema and validate-data commands
  • Updated error message to indicate retries were already attempted
  • Shared RetriesOption and RetryWaitOption type annotations prevent drift

• Plugin updates:

  • All plugins (PermissibleValuePlugin, DynamicEnumPlugin, BindingPlugin) accept and pass through service_retries and service_retry_backoff
  • DynamicEnumPlugin._traverse() wraps graph traversal operations with retry logic

• Configuration model (models.py):

  • Added service_retries and service_retry_backoff fields to ValidationConfig

• Comprehensive test suite (test_service_retry.py):

  • 391 lines of tests covering retry policy behavior, configuration precedence, lookup resilience, adapter construction, graph traversal, and end-to-end CLI validation
  • Tests verify that only transient failures are retried (missing terms are not)
  • Tests confirm backoff delays are applied correctly

Implementation Details

• Retry scope: Only OntologyServiceUnavailableError is retried; definitive answers (404, validation errors, config errors) are raised immediately
• Backoff strategy: Deterministic exponential backoff (1s, 2s, 4s, ...) with no jitter, suitable for sequential lookups from a single process
• Cache safety: Failed lookups do not poison the label cache; only successful resolutions are cached
• Configuration precedence: Explicit CLI arguments > oak_config.yaml > built-in defaults (consistent with Not4Curation behavior)
• Testability: OntologyAccess._sleep is injectable for tests to skip actual waiting

Fixes monarch-initiative/dismech#10396 where a single stalled ontology service request would fail an entire CI build despite valid data.

https://claude.ai/code/session_01A2XfAMx1VLZvHbfgCmCjvV

[claude]

Claude finished @cmungall's task in 4m 42s —— View job

---

PR review: retry transient ontology service failures

• Gather context (diff, CLAUDE.md)
• Review retry infrastructure in oak_utils.py
• Review CLI / plugin / model plumbing
• Review tests
• Post review

This is a well-scoped change. It retries only OntologyServiceUnavailableError, so a 404 or a bad adapter string still fails immediately. The precedence rules (explicit > oak_config.yaml > default) follow the Not4Curation pattern, and a failed lookup never ends up in _label_cache or _ols_term_cache because the exception is raised before the assignment. Nested retries also can't stack up: get_adapter is resolved before the lookup retry starts.

I found one gap that can stop the retry from ever running during graph traversal, plus a few smaller points.

---

1. A lazy traversal result can escape the classifier, so it is never retried (likely)

In plugins/base.py:936-943, only the method(...) call is inside try:

try:
    results = method([start_curie], **kwargs)
except ...:
    raise_if_service_unavailable(start_curie, e)
    raise
values = set(results or [])   # <- iteration happens here, outside the try

Many OAK ancestors/descendants implementations are generators, so the HTTP requests only run when set(results) iterates. A requests.ReadTimeout raised there stays a raw requests exception and is never turned into OntologyServiceUnavailableError. That has two effects:

• _traverse never retries it, which undoes the traversal half of this PR.
• In the greedy expand_enum path it surfaces as a traceback rather than "unable to validate". In _is_value_reachable it is caught by the generic except and quietly read as "not reachable".

test_graph_traversal_survives_a_transient_timeout misses this because its fake ancestors raises eagerly and returns a set. The gap existed before this PR, but this PR is the first thing that depends on it.

Suggested fix: move values = set(results or []) inside the try. Also add a test whose fake ancestors is a generator that raises partway through.

Fix this →

2. HTTP 429 gets the same short backoff as a timeout

raise_if_service_unavailable classifies 429 as transient, and the retry then waits the fixed 1s/2s. A rate-limited client usually needs longer than that, and OLS may send a Retry-After header. Worth considering: when the wrapped exception's response.headers has Retry-After, wait max(delay_for(attempt), Retry-After) (with a cap). This is optional, not a blocker.

3. Retrying adapter construction can repeat a large download

_build_adapter now retries get_adapter(...). For sqlite:obo:<prefix> that can mean re-downloading a database of several hundred MB, up to 3 times. That is probably acceptable. Please check, though, that a timeout mid-download doesn't leave a truncated .db in the pystow cache. If it did, the next attempt would "succeed" on a corrupt file and fail later with a confusing error.

4. Smaller points

• tests/conftest.py:26-35: instant_retry_backoff patches oak_utils.DEFAULT_SERVICE_RETRY_BACKOFF, but RetryPolicy.backoff's dataclass default was fixed to 1.0 at import. The patch only reaches resolve_retry_policy(), which is every OntologyAccess path. A future test that calls retry_on_service_unavailable(op) without policy/sleep would really sleep. Either reword the docstring ("for every OntologyAccess") or also patch time.sleep in oak_utils.
• oak_utils.py parse_retry_config: the two service_retry_backoff branches raise the same message and can be merged into one condition.
• Worst case with retries on: during a real outage, the first failing lookup costs about backoff × (2^retries − 1) seconds (3s by default) before the run aborts. Fine as a default, but a large --retries grows exponentially, e.g. --retries 10 ≈ 17 min. Consider capping the per-attempt delay (e.g. 30–60s) or saying so in the help text.
• The CLI message "The lookup was already retried" also prints for --retries 0, where no retry happened. You could print it only when retries > 0.

Security / performance

• No security concerns. Nothing new is read from untrusted input apart from two numeric config keys, and both are validated (bool is rejected correctly).
• The happy path costs nothing extra: one closure and a single loop iteration per lookup.

Test coverage

Coverage is good. It checks policy semantics, config precedence, bad config values, that the cache isn't filled on failure, that a 404 is not retried, the backoff timing, and label, alias, obsolescence, adapter-construction and traversal lookups, plus the CLI end to end. Gaps:

• a lazy/generator traversal (see Bump actions/setup-node from 3 to 6 #1)
• validate-data through the CLI (only validate-schema is exercised)
• the OLS _ols_is_obsolete / _ols_entity_aliases retry paths

I did not run the test suite: installing dependencies (uv sync) needs approval in this environment. If you want CI-style runs from this bot, add Bash(uv sync:*) / Bash(uv run:*) to --allowedTools.

[claude]

Claude finished @cmungall's task in 49s —— View job

---

Re-review of PR #74 after 276e5aa

• Gather context (diff of 276e5aa)
• Check how the previous review's findings were addressed
• Review the new code (Retry-After, delay cap, attempts count)
• Post review

The new commit fixes every point from the last review. I found nothing blocking. Two small follow-ups are below.

Previous findings

#	Finding	Status
1	A generator traversal could raise outside the classifier	✅ Fixed. set(method(...) or []) is now inside the try (plugins/base.py:942), and a new generator-based test covers it
2	429 ignored Retry-After	✅ retry_after_seconds() reads the delta-seconds form, and the result is clamped
3	A timed-out download could leave a truncated .db	✅ Not a problem: pystow's clean_on_failure=True deletes the partial file, as the commit message says
4a	The fixture only covered OntologyAccess paths	✅ The no-policy path now uses resolve_retry_policy(), and the docstring was updated
4b	Two duplicate backoff checks	✅ Merged into one
4c	Waits grew without limit for a large --retries	✅ Each wait is capped at MAX_SERVICE_RETRY_DELAY = 60s
4d	The error message said "already retried" even with --retries 0	✅ It now says "after N attempt(s)", using the count from exc.attempts

New code

retry_after_seconds / the delay clamp (oak_utils.py:301-337, :387-392)

• raise_if_service_unavailable raises with from exc, so _exception_chain reaches the requests error and its response. The test builds its wrapper the same way.
• min(max(delay, asked_for), max(MAX, backoff)) does what the comment says. A header asking for longer wins, an outlandish value is capped, and an explicitly larger backoff is never shortened. The parametrized test covers all four cases.
• Reusing _exception_chain in is_connectivity_error keeps the raise … from None behaviour, so that refactor is safe.

attempts on the exception

• test_the_outage_message_claims_no_retry_when_there_was_none passes --retries 0 and checks the message says "after 1 attempt." rather than "attempts".
• One edge case: if a retry loop ever wrapped another one, the outer loop would overwrite attempts with its own count. Nothing nests today, because get_adapter is resolved before the lookup retry starts. No change needed.

Small follow-ups (optional)

1. The OLS descendant fallback is neither retried nor classified. _ols_descendants (plugins/base.py:1214-1240) calls client.get_paged(...) directly, and _expand_reachable_from calls it outside _traverse. If that paged OLS request times out, the raw requests error reaches greedy expand_enum as a traceback. It is not reported as "unable to validate", and it is not retried. This is the same kind of gap as Bump actions/setup-node from 3 to 6 #1, on the path OLS-backed dynamic enums actually use when descendants() returns nothing. It is older than this PR. The fix is to run the fallback through self.ontology.retry_service_call(...), with the same try/raise_if_service_unavailable guard (which must include iterating records, since get_paged is a generator).
   Fix this →
2. validate-data is still not tested through the CLI. The CLI end-to-end tests only run validate-schema. One validate-data case with a flaky adapter would show that --retries/--retry-wait reach DynamicEnumPlugin/BindingPlugin.

Security / performance

• No security concerns. Retry-After comes from a server response, but it is parsed with float() and clamped, so a hostile or broken header cannot stall a run for more than 60s per attempt.
• There is no extra cost when lookups succeed. Worst case during an outage with the defaults is 1s + 2s per failing lookup. Because the CLI stops at the first unrecoverable failure, that cost is paid only once.

Test coverage

The new tests cover the generator traversal, Retry-After (longer / shorter / clamped / HTTP-date), the recorded attempt count, the OLS is_obsolete/entity_aliases retry paths, and the CLI message with and without retries. The only gaps are the two follow-ups above.

I didn't run the test suite: installing dependencies (uv sync) needs approval in this environment.

[cmungall]

Review addressed in 276e5aa.

1. Lazy traversal result escaping the classifier — confirmed and fixed. _call_graph_traversal now materializes inside the guard (values = set(method([start_curie], **kwargs) or [])). Verified the failure mode directly: the new test_graph_traversal_retries_a_lazily_raised_timeout (fake ancestors is a generator that raises after its first yield, on the first two calls) fails with a raw requests.exceptions.ReadTimeout on the previous code and passes now.

2. Retry-After — implemented. retry_after_seconds() walks the cause chain for a Retry-After header and the wait becomes min(max(backoff_delay, retry_after), cap), so a rate-limited service asking for longer wins but an outlandish value cannot hang the run. Only the delta-seconds form is read; the HTTP-date form is ignored rather than guessed at. Parametrized test covers all four cases (asks longer / asks shorter / absurd / date form).

3. Retried download leaving a truncated .db — checked, not an issue. pystow.utils.download() defaults to clean_on_failure=True, and its except (Exception, KeyboardInterrupt) branch does path.unlink(missing_ok=True) before re-raising, so a timed-out download leaves nothing for the next attempt to "succeed" on. No change made; noted in the commit message.

4. Smaller points — all taken.

• retry_on_service_unavailable now resolves its default via resolve_retry_policy(), so the module defaults (and the fixture that patches them) reach the no-policy path too; the fixture docstring now names exactly what it covers.
• The duplicate service_retry_backoff branches are merged into one condition.
• A single wait is capped at MAX_SERVICE_RETRY_DELAY (60s), so --retries 10 is bounded instead of ~17 minutes; a backoff explicitly set above the cap is still honored. Documented in --retry-wait help, docs/configuration.md and CLAUDE.md.
• The outage message no longer claims a retry that did not happen: the retry loop records the attempts it spent on the exception and the CLI prints "after N attempt(s)", so --retries 0 reads truthfully. Two tests pin both wordings.

Test-coverage gaps: added the OLS payload paths (is_obsolete and entity_aliases through _ols_term_dict) plus the generator traversal. I left validate-data end-to-end out: it reaches the service through the same OntologyAccess and _traverse that the new tests cover directly, so a second CLI lane would restate rather than extend the coverage.

Also folded into this push: is_connectivity_error and retry_after_seconds now share one _exception_chain() walk, with the raise ... from None semantics unchanged (its regression test still passes).

Local checks before pushing: pytest (396 passed), doctests (47), mypy src tests, ruff check . — all clean.

---

Generated by Claude Code

[cmungall]

Both re-review follow-ups handled in 971d62e.

1. OLS descendant fallback — fixed rather than deferred. You're right that it's the same gap as #1 on the path OLS-backed enums actually take, so leaving it would make this PR's claim only half true. _ols_descendants now guards both the get_paged call and the iteration of its pages with raise_if_service_unavailable, and _expand_reachable_from invokes it through self.ontology.retry_service_call(...). Two tests added: one where get_paged is a generator raising mid-iteration on the first two calls (expansion succeeds, 3 calls), one where it always raises (surfaces as OntologyServiceUnavailableError, not a raw traceback). Both were checked against the unwired code — the retry test fails there and passes now.

2. validate-data end-to-end — still declining, deliberately. That lane reaches the service through the same OntologyAccess.retry_service_call and _traverse that the unit tests now exercise directly, including the OLS paging path above. A second CLI invocation would re-run the same code behind more fixture setup, so it would restate coverage rather than extend it. If you'd rather have the belt-and-braces lane anyway, say so and I'll add it.

On your note about nested retry loops overwriting exc.attempts: agreed that nothing nests today (adapter construction resolves before the lookup retry begins, and the fallback runs in place of _traverse, not inside it). If a future call site ever nests, the inner loop's count would be the one overwritten — worth remembering, but no guard added for a case that cannot currently arise.

Local checks before pushing: pytest (398 passed), doctests (47), mypy src tests, ruff check . — all clean.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 28s —— View job

---

Re-review of PR #74 after 971d62e

• Read the diff of 971d62e
• Check the fix against the earlier follow-ups
• Post review

This commit fixes follow-up #1 from the last review. I found nothing blocking, and I think the PR is ready to merge.

What changed

_ols_descendants (plugins/base.py:1233-1252)

• The try block now covers both the client.get_paged(...) call and the set comprehension that reads its pages. That matters because get_paged is a generator: the page requests actually run while the set is being built, so a guard around the call alone would miss them. This is the same fix _call_graph_traversal got earlier.
• If an OntologyServiceUnavailableError is already raised, it goes straight through, so it isn't wrapped twice. Any other error goes through raise_if_service_unavailable. Errors that aren't about the service being down, such as a KeyError from a malformed payload, still surface unchanged, which is correct.
• _ols_descendants has only one caller, in _expand_reachable_from, so no other call site needs this fix.

The call site (plugins/base.py:1187-1198)

• The call now goes through self.ontology.retry_service_call(...), so it follows the same retry policy and records the same attempts count as _traverse.
• The lambda uses the loop variable source_node. That would be a bug if the lambda ran later, but retry_service_call calls it straight away, so it is safe.
• The two retries don't stack. _traverse("descendants") finishes first, and the fallback only runs if it returned nothing. In the worst case the two steps each spend their own retries one after the other, and neither retry loop wraps the other.

Tests

• test_ols_descendant_fallback_retries_a_lazily_raised_timeout: the fake get_paged yields one record and then raises. It fails twice and succeeds on the third call. The test checks both the full result and calls == 3, which confirms the retry re-fetches the whole page set and doesn't keep the half-read one.
• test_ols_descendant_fallback_classifies_a_persistent_timeout: when the timeout never clears, the test expects OntologyServiceUnavailableError rather than a raw requests traceback.

Minor notes (optional)

• A retry fetches every page again. If a timeout hits page N of a large descendant set, the next attempt starts again from page 1. For a single run that is acceptable. If very large OLS closures become common, resuming from the failed page would be a later optimisation.
• Still open from last time: validate-data has no CLI end-to-end test. You've explained why you left it out: it uses the same OntologyAccess and _traverse paths that are tested directly. That's a reasonable call.

Security / performance

• No new concerns. There is no extra cost when requests succeed. Retries only happen on the OLS fallback path, and only when a request actually fails.

Test coverage

The paths the PR now covers are all tested: label, obsolete and alias lookups (both OAK and OLS payloads), adapter construction, eager and lazy traversal, the OLS paging fallback, Retry-After, and the CLI messages.

I didn't run the test suite here, because installing dependencies (uv sync) needs approval in this environment. The author's comment says pytest, doctests, mypy and ruff passed locally for the previous push, and CI is the place to confirm them for this one.