Bound PubMed transport retries and preserve cached text on failure

[cmungall]

A dropped NCBI response could abort validation, leak a response handle, or leave only summary metadata available after an article fetch failed. Read complete summary/article responses with bounded transport retries, close handles on every exit, and return None after exhaustion so validation can continue and eligible stale cache entries remain useful without being overwritten.

Behavior

• Retry direct connection failures, socket/TLS errors, and HTTP framing errors (including incomplete reads) up to three attempts with 2/4 second backoff. Keep XML parsing outside that retry boundary.
• Preserve Bio.Entrez's opening retry policy: exhausted HTTP/URL errors do not restart its loop, permanent HTTP 4xx errors are not retried, and no global retry settings are changed. Mixed successful opening retries followed by body failures have a tested maximum of nine HTTP requests per endpoint under Bio.Entrez defaults.
• Report unfetchable references as unsuccessful validation, continue with subsequent references, and preserve stale files on partial refresh failure (including forced refresh).
• Clarify Offline stale caches retry every reference on every run; clarify refresh cost #75's repeated per-reference offline cost and that migration completes only after successful refresh. No circuit breaker is added.

The legacy PMC fetch methods covered by #64 are unchanged. A successfully fetched record that genuinely has no abstract still returns metadata with unavailable content.

Validation

• TDD: the first local HTTP regression run reproduced 12 failures / 3 passes before implementation; the review follow-up reproduced 8 TLS/HTTP-framing failures before fixing them.
• Final controlled HTTP/socket regression suite: 41 passed, covering disconnect/truncation recovery and exhaustion, opening/body timeouts, actual connection refusal, HTTP 400/429/503, mixed retry bounds, handle cleanup, deterministic parsing, subsequent validation, and stale cache preservation.
• Final network + existing source tests: 99 passed on 722a567.
• just doctest: 200 passed.
• uv run mypy src tests: passed; uv run ruff check .: passed; git diff --check: passed.
• just test on 722a567: 840 passed (354.97 seconds), followed by passing mypy and ruff checks.
• GitHub CI on Python 3.10, 3.11, 3.12, and 3.13 passed on final commit 722a567: Build and test run. No failed-check retries were needed.
• Follow-up automated review completed with no merge blockers; final disposition records the remaining scope/compatibility decisions.

Retry limits bound attempts, not elapsed wall time; Bio.Entrez retains its own opening delays. The #65 HTML-cache exception belongs to its separate PR and may require combining nearby troubleshooting edits when both land.

Closes #66
Closes #75

[claude]

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

---

Review: bounded PubMed transport retries

• Gather context (read diff and changed files)
• Review source changes in pmid.py
• Review test coverage in tests/test_pmid_network.py
• Review docs changes
• Post review feedback

Overall this is a solid, well-reasoned change. The retry boundary is in the right place (transport only, parsing outside), finally: handle.close() fixes the handle leak, deliberately not wrapping Bio.Entrez's own retry loop is the right call, and the test file exercises real sockets rather than mocks — exactly what CLAUDE.md asks for. The sleeps-list assertions that prove 2/4 backoff is absent on the connection-refused path (tests/test_pmid_network.py:260) are a nice touch: they test the negative, not just the positive.

Comments below, roughly in severity order.

---

1. A successful-but-empty efetch response is treated as a transport failure

_fetch_pubmed_xml returns None for two very different situations: transport exhausted, and a complete 200 response whose body is empty (src/linkml_reference_validator/etl/sources/pmid.py:309). fetch() then returns None for both (pmid.py:153-156), so a record that NCBI genuinely serves as an empty efetch body is reported as unfetchable forever, and the good summary metadata (title, authors, journal, DOI) is thrown away.

The same applies to permanent errors: test_entrez_open_retries_are_not_multiplied shows efetch HTTP 400 → whole reference None. Before this PR that case yielded summary-only content; now the reference can never be validated even on a title-only check, and no amount of retrying will change a 400.

The stale-cache motivation only really needs the None for the transport-exhausted case. Consider distinguishing them — e.g. _read_entrez returning a sentinel (or _fetch_pubmed_xml raising a small internal TransportExhausted) so fetch() can return None on exhaustion but degrade to summary-only content on a complete-but-empty/permanently-rejected article response. There's also no test for the empty-200 efetch body today.

Fix this →

2. The read-path except tuple can still let a network error abort the run

pmid.py:276 catches IncompleteRead, RemoteDisconnected, ConnectionError, TimeoutError, URLError. Real NCBI traffic is HTTPS, and a mid-body TLS failure raises ssl.SSLError / ssl.SSLEOFError — an OSError subclass but neither ConnectionError nor TimeoutError, so it escapes the retry loop and propagates out of fetch(), aborting validation. That's the exact failure mode #66 is about. Sibling http.client body errors (BadStatusLine, LineTooLong, chunked-decode failures) escape too.

except (OSError, HTTPException) is simpler and strictly broader: ConnectionError, TimeoutError, URLError and ssl.SSLError are all OSError; IncompleteRead and RemoteDisconnected are covered by HTTPException/ConnectionError.

Two nits in the same tuple: RemoteDisconnected is already a ConnectionResetError, and URLError is unreachable here — the inner except URLError at pmid.py:268 returns first, and handle reads don't raise URLError. The overlap between the two tuples makes the control flow harder to follow than it needs to be.

Fix this →

3. Sub-second timeout margins plus exact request-count assertions look flake-prone

The fixture opens local handles with timeout=0.2 (tests/test_pmid_network.py:88) while the "timeout"/"read_timeout" actions stall 0.4s. On a loaded CI runner a legitimate local response can exceed 200 ms, which turns into an extra attempt — and every test asserts an exact count (counts[endpoint] == 3, == 9). The margin between "intended timeout" and "incidental slowness" is only 200 ms.

Widening to something like timeout=1.0 with 3s stalls costs a handful of seconds across the parametrized cases and removes the sensitivity to scheduler jitter. Worth doing given how many tests depend on it.

4. monkeypatch.setattr(Entrez.time, "sleep", ...) patches the stdlib time module globally

Entrez.time is the time module, so tests/test_pmid_network.py:93 replaces time.sleep process-wide for the duration of the test — including pmid.py's own time.sleep calls. That's evidently intentional (it's how 2 and 4 land in sleeps), and it's also why the handler has to use Event().wait(0.4) instead of time.sleep. But the comment above it reads as if only Entrez's sleeps are patched, and the sleeps list silently mixes three sources: Entrez's sleep_between_tries, the validator's backoff, and config.rate_limit_delay zeros. A one-line comment saying "this is the stdlib time module — it patches the validator's sleeps too, hence Event().wait in the handler" would save the next reader a debugging session.

sleeps.count(Entrez.sleep_between_tries) at line 259 is also silently coupled to that default not being 2 or 4, given the assertion on the next line.

5. Minor code cleanups

• pmid.py:157-161: after the article_xml is None check at line 153, the three if article_xml else None guards are effectively dead. They aren't provably dead (an empty Tag is falsy via __len__), which makes them more confusing than protective — worth simplifying with a short comment if the falsy-soup case actually matters.
• pmid.py:262,285: the attempt count 3 and 2 ** (attempt + 1) are magic numbers repeated in the docstring, the log message (attempt %s/3) and the docs. Module constants (_MAX_TRANSPORT_ATTEMPTS, _BACKOFF_BASE_SECONDS) would keep them in sync.
• _read_entrez's docstring skips the Args:/Returns: sections every other method in the file uses.
• pmid.py:276 is ~100 chars and the signature at 250-252 wraps unusually; ruff's default rule set doesn't include E501, so this passes lint but reads inconsistently with the rest of the file.
• _fetch_pmc_xml (pmid.py:476-479) still does unguarded handle.read() with no close() on error — the same leak this PR fixes elsewhere. I see it's deferred to Remove dead PMC full-text code from PMIDSource #64, but since _read_entrez takes a callable, routing it through is nearly a one-liner and would close the leak now.

6. Test nit: hardcoded cache version

tests/test_pmid_network.py:178 rewrites "extractor_version: 1". EXTRACTOR_CACHE_VERSION exists in reference_fetcher.py — using f"extractor_version: {EXTRACTOR_CACHE_VERSION}" keeps the test honest when the version bumps (it would fail loudly rather than pass for the wrong reason, but the failure would be baffling).

7. Worth confirming: does Entrez.read reach the network in these tests?

The fixture patches Bio.Entrez.urlopen, but Bio.Entrez.Parser has its own urlopen for resolving the DOCTYPE. SUMMARY declares the NCBI esummary-v1.dtd; if that DTD isn't bundled/cached locally, Entrez.read will make a real outbound request from a suite advertised as fully controlled. Probably fine (Biopython ships common DTDs), but worth verifying the suite passes with outbound network blocked — and it's a reason the (ValueError, RuntimeError) catch at pmid.py:125 may not cover everything the parse step can raise.

8. Wall-clock cost (agreeing with the docs, flagging for follow-up)

With Bio.Entrez defaults (max_tries=3, sleep_between_tries=15), a fully unreachable PubMed costs roughly 3 × (2 × 15s) + 6s of local backoff ≈ 96s per endpoint, so ~3 minutes per PMID, repeated for every distinct reference with no circuit breaker. docs/troubleshooting.md describes this honestly, which I appreciate — but for a large offline run the practical effect is an unbounded-feeling hang. A per-run consecutive-failure short-circuit (or a wall-clock deadline in config) would be a worthwhile follow-up issue; I agree it doesn't belong in this PR.

---

Things I could not verify

The sandbox for this run denied uv run, so I could not execute the test suite, mypy, ruff, or just test — the review above is static analysis only, and I'm taking the PR description's reported results at face value. The description notes just test was still running when the PR was opened; worth recording its final result.

Nothing here is a blocker except possibly #1 and #2, both of which are about behavior at the edges rather than the core design, which I think is right.
· branch cmungall/p1-66-network

[cmungall]

Review disposition in 722a567:

• Fixed TLS/HTTP framing coverage with (OSError, HTTPException) strictly around opening/reading, retaining the separate exhausted-Entrez URLError path and keeping XML parsing outside retries. Eight new recovery/exhaustion regressions failed before the fix; the TLS case injects SSLEOFError after consuming bytes from a real local HTTP response, and malformed HTTP status lines come from the controlled server.
• Widened local timeouts to 1 second / 3-second stalls, documented the shared sleep patch, used EXTRACTOR_CACHE_VERSION, and made any external DTD request fail the fixture. The full network/source suite passes with that guard: 99 tests; 200 doctests, mypy, and ruff also pass.
• Retained None for empty or rejected article responses: summary-only refreshes could overwrite useful stale text. A new empty-200 test verifies preservation without retrying that complete response. A successful article XML record with no abstract still returns metadata as unavailable; HTTP 400 on the base branch raised from efetch rather than returning summary-only content.
• Legacy PMC methods remain owned by Remove dead PMC full-text code from PMIDSource #64; circuit breaking remains out of scope. The 96-second estimate does not apply to a fully unreachable endpoint: an exhausted Bio.Entrez opening loop returns immediately without an outer restart; mixed opening/body failures have the separately documented bound.

The full local suite and Python 3.10–3.13 CI are running on the updated commit; final results will be recorded in the PR description.

[claude]

Claude finished @cmungall's task in 5m 3s —— View job

---

Review: bounded PubMed transport retries (pass 2, after 722a567)

• Gather context (diff vs origin/main, changed files)
• Review pmid.py transport changes
• Review tests/test_pmid_network.py
• Review docs changes
• Check which prior-review points the new commit addresses
• Post review

The follow-up commit closes the two items I'd flagged as the only near-blockers. except (OSError, HTTPException) at src/linkml_reference_validator/etl/sources/pmid.py:286 is now broad enough that a mid-body TLS error or an HTTP framing error retries instead of escaping fetch(), and the new tls_eof / bad_status handler actions exercise both through a real socket. TLSFailureHandle consuming 20 real bytes before raising is a better harness than an OpenSSL-behaviour mock would have been, and appending the inner handle to handles means all(handle.closed ...) still proves real cleanup through the wrapper's __getattr__.

Also resolved since the last pass: EXTRACTOR_CACHE_VERSION instead of a hardcoded 1, the Entrez.time comment at tests/test_pmid_network.py:130, the widened timeout=1 / 3 s stall margin, Args:/Returns: on _read_entrez, and — nicely — monkeypatch.setattr(Parser, "urlopen", forbid_dtd_network) (tests/test_pmid_network.py:124-128), which turns my "does the parser reach the network?" question into an assertion.

Remaining comments, severity order.

---

1. fetch() still conflates "transport exhausted" with "complete response, no article"

pmid.py:152-156 returns None for every falsy _fetch_pubmed_xml result. The stale-cache tests cover the case where a cache entry exists — _stale_fallback (reference_fetcher.py:182-211) rescues those. But that fallback is only reachable when a cache entry already exists on disk. For a first fetch of a PMID where esummary succeeds and efetch permanently fails (HTTP 400 — see test_entrez_open_retries_are_not_multiplied, which asserts fetch(...) is None for efetch 400 — or a complete 200 with an empty body), the reference is now reported as not found, where before it degraded to summary-only content. A title-only validation that used to pass now fails, and no retry will ever change it.

The stale-preservation motivation only needs None for the exhausted case. A sentinel from _read_entrez (or a small internal TransportExhausted) would let fetch() return None on exhaustion and fall through to summary-only content on a complete-but-unusable article response. I raised this last round and see the deliberate comment at pmid.py:154-155; flagging once more only because the uncached path is the one the current tests don't cover, then dropping it.

Fix this →

2. BytesIO(summary) commits to bytes, but the codebase disagrees about what Entrez returns

pmid.py:124 does Entrez.read(BytesIO(summary)). _read_entrez is annotated Optional[bytes], but it returns handle.read() — Any from an untyped Bio stub, so mypy validates nothing here; the annotation launders the assumption rather than checking it.

Three places in this repo describe that same handle.read() differently:

• pmid.py:321-327: "decoding bytes here assumes UTF-8" → bytes
• pmid.py:~485 (_fetch_pmc_xml): "Entrez returns str"
• etl/fulltext/pmc.py:102: Optional[Union[bytes, str]] — explicitly either

Under biopython 1.86 (the lock) it's bytes and everything works. But pyproject.toml:20 allows >= 1.80, and if any supported version hands back str, BytesIO(str) raises TypeError outside every handler in the chain and aborts the run — the exact failure shape #66 is about, reintroduced at the parse boundary. Either verify and tighten the floor, or have _read_entrez normalise once and delete the now-stale comment. Worth doing regardless just so the module stops contradicting itself.

Fix this →

3. Transport hardening is PMID-source-only; the live full-text path still reads unguarded

etl/fulltext/pmc.py:110-113 is the production PMC path (PMIDSource._fetch_pmc_fulltext has no caller in src/ — only tests reach it, so #64 may be able to delete it outright rather than fix it). It still does Entrez.efetch → handle.read() → close() with no try, so an IncompleteRead there leaks the handle and loses the article. It can't abort the run — _enrich_with_full_text's except Exception at reference_fetcher.py:286-290 catches it and sets had_error — but the outcome is "full text silently skipped" on exactly the transient failure _read_entrez was built to recover. _read_entrez takes a callable, so routing this through it is close to a one-liner and would make the retry policy uniform across NCBI traffic. Fine as a follow-up, but worth naming in #64 rather than leaving implicit.

4. An empty summary body is reported as a parse error

_read_entrez returning b"" is "success" for the summary (summary is None check, pmid.py:118) but "no content" for the article (if not xml_content, pmid.py:319). A 200 with a zero-length body is more likely a truncation symptom than malformed XML, yet it logs Failed to parse PMID:x summary: ... — which points whoever debugs it at the parser instead of the network. A length check before Entrez.read would put it in the right bucket.

5. Still-open nits from the last pass

• Magic numbers. range(3) / attempt < 2 / 2 ** (attempt + 1) / "attempt %s/3" (pmid.py:272-295) are repeated across the code, the docstring, and docs/troubleshooting.md:386-390. Two module constants would keep the three in sync when someone retunes this.
• Dead guards. After the article_xml is None return at pmid.py:153, the three if article_xml else None at pmid.py:157-161 are unreachable for None — they only fire for a falsy-but-not-None BeautifulSoup. Simplify, or say in a comment which case they're for.
• Docs wrap. docs/troubleshooting.md:386-387 breaks mid-phrase ("with 2 and 4 second / backoff"); cosmetic only.

6. Test nits

• test_connection_refused_uses_only_entrez_retries:302-303 asserts sleeps.count(Entrez.sleep_between_tries) == 2 and then 2 not in sleeps and 4 not in sleeps. Those two lines quietly contradict each other if biopython ever defaults sleep_between_tries to 2. You already pin max_tries in the fixture — pinning sleep_between_tries to a value that can't collide with the backoff constants would make the pair self-consistent.
• Parse-failure coverage only exercises the ValueError branch (invalid → NotXMLError). Entrez.read also raises RuntimeError for an eutils <ERROR> element, which is the other half of the except (ValueError, RuntimeError) at pmid.py:125 and the more likely real-world one.

---

Things I could not verify

uv run is still denied in this sandbox, so this is static analysis only — no pytest, mypy, ruff, or just test from me. The Build and test workflow for this push was still in_progress when I wrote this (run 34921120662); worth checking it went green, particularly the 24 parametrized transport cases, which are the ones most exposed to runner jitter.

Nothing here blocks merge. The design is right and the tests are the kind that would actually have caught #66.
· branch cmungall/p1-66-network

[cmungall]

Follow-up review assessed; no further code changes are needed for this PR. Biopython's minimum supported 1.80 implementation already documents that XML responses are returned in binary mode and that Entrez.read requires binary input; _open wraps only plain-text responses. The locked 1.86 implementation has the same behavior, so the unrelated legacy PMC comment does not establish an XML-summary compatibility gap, and arbitrary re-encoding could damage the declared XML encoding.

The empty/rejected article behavior remains the deliberately chosen stale-safe fetch contract described above; HTTP 400 previously raised, and expanding title-only/metadata-only refresh behavior would need separate cache-quality semantics. Live PMC provider retries and the remaining cosmetic/test nits are outside this bounded fix.

Final local evidence at 722a567: 840 tests passed via just test, including passing mypy/ruff, plus 200 doctests and 99 focused network/source tests. The follow-up automated review completed with no merge blockers; Python 3.10–3.13 CI is still running.