Honour Scryfall rate limiting when fetching card images on desktop - #11524
Open
liamiak wants to merge 2 commits into
Open
Honour Scryfall rate limiting when fetching card images on desktop#11524liamiak wants to merge 2 commits into
liamiak wants to merge 2 commits into
Conversation
The mobile fetcher checks the HTTP status when downloading a card image and, on a 429, backs off Scryfall for five minutes. Desktop never got that: it calls ImageIO.read(URL), which throws the response code away, so a rate limit looks like any other failed download and we carry on asking. The cooldown field it would use is already declared on the shared base class, so only the desktop half was ever wired up. SwingImageFetcher now reads through a URLConnection, sends the User-Agent and Accept headers Scryfall asks for, and records the cooldown on a 429. The cooldown logic moves to ImageFetcher so both platforms share one copy rather than mobile carrying its own. Desktop also had no spacing between requests. Downloads are submitted to a work stealing pool, so a screen of missing images - a deck editor grid, for instance - asks for all of them at once. paceScryfall keeps a 100ms gap, the same interval GuiDownloadService already keeps for its bulk downloads. Mobile already sleeps between attempts in its own loop and is left alone. The pool this sleeps in has two users, both image related, so pacing delays other image work and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
thanks, finally respecting their limits will be good 👍 |
Nothing ever removes a path from the in-flight set, so a fetch registered and then abandoned inside the download task stays registered - the image would never be retried once the cooldown lifted, which is worse than not backing off at all. Checking in setupObserver instead means the path is never registered, so it is picked up normally next time something asks for it. Only when every candidate URL is Scryfall; if another source might still serve the image there is no reason to skip it. Splits the cooldown check in two so the pre-registration test does not log per URL, leaving the logging one for the download task. 351 tests, 0 failures; desktop and mobile both build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
4 participants
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.
The mobile fetcher checks the HTTP status when downloading a card image and, on a 429, backs off
Scryfall for five minutes. Desktop never got that - it calls
ImageIO.read(URL), which throws theresponse code away, so a rate limit is indistinguishable from any other failed download and we
carry on asking. The cooldown field it would use is already declared on the shared base class;
only the mobile half was ever wired up, in 678aae4 back in February.
SwingImageFetchernow reads through aURLConnection, sends the User-Agent and Accept headersScryfall asks for, and records the cooldown on a 429. The cooldown logic moves up to
ImageFetcherso both platforms share one copy rather than mobile carrying its own.
Desktop also had no spacing between requests. Downloads are submitted to a work stealing pool, so a
screen of missing images - a deck editor grid, for instance - asks for all of them at once.
paceScryfallkeeps a 100ms gap, the same intervalGuiDownloadServicealready keeps for its bulkdownloads. Mobile already sleeps between attempts in its own loop, so it is left alone.
The pool this sleeps in has exactly two users,
ImageKeysset lookup andImageFetcheritself, sopacing delays other image work and nothing else.
Not unit tested: the cooldown is static mutable state shared across the JVM, and setting it from a
test would leak a five minute Scryfall cooldown into the rest of the suite. Verified by build on
both the desktop and mobile modules, and the desktop suite - 351 tests, 0 failures.
Written with Claude Opus 5 (also recorded in the commit co-authors).