Skip to content

fix(literature): don't sleep after the final Semantic Scholar retry attempt#305

Open
atulya-singh wants to merge 1 commit into
aiming-lab:mainfrom
atulya-singh:fix/s2-retry-final-sleep
Open

fix(literature): don't sleep after the final Semantic Scholar retry attempt#305
atulya-singh wants to merge 1 commit into
aiming-lab:mainfrom
atulya-singh:fix/s2-retry-final-sleep

Conversation

@atulya-singh

Copy link
Copy Markdown
Contributor

Both retry loops in semantic_scholar.py (_request_with_retry and _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:

S2 request failed (boom). Retry 2/3 in 2s…
S2 request failed (boom). Retry 3/3 in 4s…     <- there is no attempt 4
S2 request exhausted retries for: ...

It announces "Retry 3/3", sleeps 4s (+jitter), and returns None. Nothing retries. Measured sleep budget on a persistent connection error, before/after:

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_query loops 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:

if exc.code == 429:
    if _cb_on_429():
        return None  # breaker tripped -> returns before the sleep

_CB_THRESHOLD is 3 and _MAX_RETRIES is 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_RETRIES to 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_attempt guard. 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 wait while actually sleeping wait + jitter, and formatted a float with %d (so a 4.25s sleep logged as 4s). 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:

FAILED TestS2RetrySleepBudget::test_get_connection_error_does_not_sleep_after_final_attempt
FAILED TestS2RetrySleepBudget::test_post_connection_error_does_not_sleep_after_final_attempt
2 failed, 3 passed

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 None return, same breaker semantics.


Same class of bug as #304 (the LLMClient retry loop), but independent code — either can land without the other.

_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.
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