fix(literature): don't sleep after the final Semantic Scholar retry attempt#305
Open
atulya-singh wants to merge 1 commit into
Open
fix(literature): don't sleep after the final Semantic Scholar retry attempt#305atulya-singh wants to merge 1 commit into
atulya-singh wants to merge 1 commit into
Conversation
_request_with_retry and _post_with_retry both slept their full backoff after the last attempt and then gave up anyway, so every exhausted call burned ~4-5s doing nothing. The log line gave it away: it printed "Retry 3/3 in 4s…" when _MAX_RETRIES is 3 and no fourth attempt exists. Only the connection-error branch (URLError/OSError/JSONDecodeError) was actually affected. The 429 branch is saved by the circuit breaker, which trips at _CB_THRESHOLD consecutive 429s and returns before reaching the sleep — but only because _CB_THRESHOLD happens to equal _MAX_RETRIES. Lower _MAX_RETRIES (or raise the threshold) and it would leak the same wasted sleep, so both branches now carry an explicit is_last_attempt guard rather than relying on the two constants staying in sync. Cost is per source per query, and search_papers_multi_query loops over both, so it multiplies out on exactly the flaky-network runs you least want to sit through. Also fixed the neighbouring log statement, which reported `wait` while sleeping `wait + jitter` and formatted a float with %d. Behaviour is otherwise unchanged: same attempt count, same backoff growth, same None return. Tests assert the sleep budget for both functions, and that backoff still grows between attempts so this can't be "fixed" by dropping the sleeps altogether.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both retry loops in
semantic_scholar.py(_request_with_retryand_post_with_retry) slept their full backoff after the final attempt, then gave up anyway. The log line is the giveaway — with_MAX_RETRIES = 3:It announces "Retry 3/3", sleeps 4s (+jitter), and returns
None. Nothing retries. Measured sleep budget on a persistent connection error, before/after:_request_with_retry[1.1, 2.2, 4.3][1.1, 2.2]_post_with_retry[1.2, 2.4, 4.3][1.2, 2.4]So ~4–5s burned per exhausted call. That's per source per query, and
search_papers_multi_queryloops over both, so it multiplies out — on precisely the flaky-network runs you least want to sit through.Only one branch was actually broken
Worth being precise, because it wasn't what I first assumed. The 429 branch is fine, and not by design — it's saved by the circuit breaker:
_CB_THRESHOLDis 3 and_MAX_RETRIESis 3, so the third consecutive 429 trips the breaker and returns early, skipping the sleep. Correct — but only as long as those two unrelated constants stay equal. Drop_MAX_RETRIESto 2, or raise_CB_THRESHOLD, and the same wasted sleep reappears with no test to catch it.Rather than leave that coupling implicit, both branches now carry an explicit
is_last_attemptguard. The 429 path still calls_cb_on_429()first, so breaker accounting is unchanged — it just skips the pointless sleep.The only branch with a live bug is the connection-error one (
URLError/OSError/JSONDecodeError), which has no breaker involvement at all.Drive-by
The connection-error log statement reported
waitwhile actually sleepingwait + jitter, and formatted a float with%d(so a 4.25s sleep logged as4s). Since I was rewriting those exact lines I folded the jitter in and switched to%.1f. Happy to pull this out if you'd rather keep the PR to one thing.Testing
pytest tests/→ 2802 passed, 56 skipped, unchanged from before.The new tests assert the sleep budget for both functions and both branches. I checked they actually fail against the unfixed code rather than just passing alongside it:
The three that pass either way are the 429 cases (never broken, per above) — they're there as guards on the constant coupling. There's also one asserting backoff still grows between attempts, so this can't be "fixed" later by just deleting the sleeps.
Behaviour is otherwise unchanged: same attempt count, same backoff growth, same
Nonereturn, same breaker semantics.Same class of bug as #304 (the
LLMClientretry loop), but independent code — either can land without the other.