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

[atulya-singh]

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.

[Jiaaqiliu]

Merged. The write-up is a model of how to report this class of bug — particularly the part where you corrected your own first assumption.

Being precise that the 429 branch was never broken, and that it's saved incidentally by _CB_THRESHOLD == _MAX_RETRIES == 3 rather than by design, is more useful than the fix itself. That's a latent bug held shut by two unrelated constants happening to be equal, and the explicit is_last_attempt guard on both branches is the right way to stop it from reopening the moment someone tunes either one. Keeping the _cb_on_429() call ahead of the guard so breaker accounting is unchanged is the correct ordering.

The drive-by is fine to keep in scope — you were rewriting those exact lines, and %d on a float genuinely misreported the sleep (4.25s logged as 4s).

Also appreciated: showing the new tests failing against unfixed code rather than just passing alongside it, and noting which three pass either way and why they're still worth having.

Post-merge: 2956 passed, no regressions.