perf: avoid duplicate IA loan API call in get_loan#13130
Draft
lokesh wants to merge 1 commit into
Draft
Conversation
get_loan() unconditionally made a second _get_ia_loan() call and overwrote the first result. This caused: - two identical blocking POSTs to the IA loan API for anonymous lookups (every edition-page loan check via get_edition_loans) - a loan found under ol:<username> being clobbered by None when the itemname lookup found nothing - an unfiltered (userid-less) lookup for accounts with no linked itemname, which could return another user's loan Only make the second call when the account has a linked itemname and the first lookup found nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ky5Sc19MGPkDgbgTWWWET
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.
Closes #
Performance fix (with a correctness fix included): removes a redundant blocking HTTP POST to the archive.org loan API from every edition-page loan check.
Estimated performance gain
~30–150 ms off server render time (p50–p90) for every anonymous edition-page loan check, and worst-case stalls halved from 2× to 1× the API timeout. The loan check is a synchronous cross-service POST on the render path; this PR halves the number of calls, so the saving is one full loan-API round-trip. Estimates assume typical same-datacenter latency to the IA loan service; measure with the existing
statstiming onget_loancallers to confirm.xychart-beta title "Blocking loan-API calls per anonymous loan check" x-axis ["Before", "After"] y-axis "sequential HTTP POSTs" 0 --> 2 bar [2, 1]For user-scoped lookups the second call still happens, but only when it can matter (linked
@itemnameand the first lookup found nothing) — previously it ran unconditionally.Technical
Problem.
get_loan()inopenlibrary/core/lending.pymade two sequential, unconditional calls to_get_ia_loan(), each a blockingrequests.postto the IA loan API, and the second call unconditionally overwrote the first's result:This had three concrete consequences:
user_key is None(the common case —get_edition_loans()inplugins/upstream/borrow.pycallsget_loan(ocaid)with no user on edition-page renders, andis_loaned_out_on_ol()does the same),accountisNone, so both calls were byte-for-byte identical POSTs. Every loan-status check paid for two round-trips to the loan API where one suffices.None, the function returnedNone— losing a real loan.account.itemnameisNone, the second call was made withuserid=None, i.e. an unfiltered query that returns any loan on the identifier — so a user-scopedget_loan(identifier, user_key)could return another patron's loan.Fix. The second lookup now runs only when it can produce new, correctly-scoped information:
if account and account.itemname and not _loan. Anonymous lookups make exactly one API call; user-scoped lookups fall back to the@itemnamequery only when theol:usernamequery found nothing.Out of scope (pre-existing, noted for reviewers): the store-loan block just above (
d = site.get().store.get("loan-" + identifier)) constructs aLoan(d)that is never returned when unexpired; that dead code predates this PR and is untouched.Testing
New
TestGetLoaninopenlibrary/tests/core/test_lending.py(mockslending.siteandlending._get_ia_loan):ol:username→ no second call, loan not clobbered@itemnamecallitemname→ exactly one callReproduce:
make test-py-uv PYTEST_ARGS="openlibrary/tests/core/test_lending.py"— 8 passed.pre-commit run --files openlibrary/core/lending.py openlibrary/tests/core/test_lending.py— all hooks pass (incl. mypy, ruff).Screenshot
N/A — no UI change.
Stakeholders
Part of a performance-review series; see the review report branch
claude/performance-review-top-changes-5wcs7g.🤖 Generated with Claude Code
https://claude.ai/code/session_015ky5Sc19MGPkDgbgTWWWET