fix(screenscraper): report rejected credentials instead of a bare 403

[Spinnich]

Fixes #4122

Description

When ScreenScraper refused the configured credentials, RomM logged 403, message='Forbidden' and carried on. Every ROM was saved without ScreenScraper metadata, the "ScreenScraper quota:" line never appeared, and nothing anywhere mentioned credentials. The report behind #4122 took hours to diagnose, most of it spent looking at the wrong thing (the account's quota), because that is where the silence pointed.

The cause was ordering. _request() already had the right message: LOGIN_ERROR_CHECK matches the Erreur de login string in the response body and raises a clear credentials error. But the check sat after res.raise_for_status(), and ScreenScraper sends that string with HTTP 403, which raised first. The check was dead code for the exact case it was written for, in both the first attempt and the retry, and 403 had no branch of its own, so it fell to the catch-all log.error(err).

This PR:

• Reads the body before raising the status. A refusal explains itself in the body, so a 403 no longer aborts the attempt with a bare "Forbidden" and loses the one line that says what is wrong.

• Says which of the two credential sets was refused, which is the one thing an operator needs in order to know where to look:

  ScreenScraper rejected your user account credentials. Check SCREENSCRAPER_USER and SCREENSCRAPER_PASSWORD.
  ScreenScraper rejected the RomM developer credentials.
  

  The developer message deliberately does not name its variables. They are only semi-protected (readable from any published image), and nothing should send an end user hunting for them; naming the set is enough to point troubleshooting at a bad build.

• Attributes the refusal from the endpoint, not from ScreenScraper's wording. ssuserInfos.php is the only endpoint that checks the account password, so a refusal anywhere else can only be about the developer credentials. ScreenScraper's own text is not a reliable signal by itself: with a bad developer id, ssuserInfos.php still answers Vérifier les identifiants utilisateurs. Its text is appended to the message rather than interpreted, so a bug report still carries the provider's exact words.

• Trips a breaker, like the daily-quota path already does. The failure is logged once per set, the remaining requests short-circuit instead of paying a round trip per ROM to be told the same thing, and the three scan-path lookups in ss_handler swallow it so the scan degrades to the other providers instead of dying.

• The account check reports, but never arms that breaker. ScreenScraper refuses a developer id it accepted a minute earlier ([Feature] Validate ScreenScraper credentials and show quota usage on the metadata sources page #4124) while the scraping endpoints keep answering, so letting prime_account_limits() leave the breaker armed cost a working scan all of its ScreenScraper metadata. Only a request the scan actually needs can take the provider out. This is [Bug] A single ScreenScraper quota error disables ScreenScraper for an entire scan, and manual matching never recovers #4123's rule, applied to the breaker introduced here; the quota breaker still needs it.

• Makes prime_account_limits() warn when the call returns empty, not just when it raises. Several errors are swallowed into an empty response, which used to leave a scan with no limits and nothing said about it.

• Gives HTTP 401 both halves of its documented meaning. ScreenScraper's error table splits that row into a description ("API fermé pour les non membres ou les membres inactifs") and a cause ("Le Serveur est saturé (utilisation CPU>60%)"). RomM reported the cause and dropped the description, so a lapsed member read a 401 as a passing server-side problem. (Item 4 of the issue originally claimed the message named the wrong cause; the issue has been corrected.)

• Collapses the duplicated request block. The two attempts each carried their own copy of the status ladder, and the copies had drifted: 401 and 403 were handled differently between them. Both now go through one _attempt() and one _handle_client_error().

Files modified

File	What changed
backend/adapters/services/screenscraper.py	New SSCredentialSet, CREDENTIAL_DETAILS and ScreenScraperCredentialsError. _credential_set() attributes a refusal; _reject_credentials() reports it once and records the set in _ScanState.credentials_rejected, so the requests that short-circuit repeat the same message. _attempt() holds the single request path and reads the body before raise_for_status(); _handle_client_error() is the one status ladder both attempts share. _error_message() condenses the provider's reply to a single line and masks anything credential-shaped. prime_account_limits() warns on an empty result as well as on a raise, and clears the breaker so the account check can never disable the provider. The 401 message carries the documented description and cause.
backend/handler/metadata/ss_handler.py	New _is_provider_exhausted() covers both the exhausted quota and the refused credentials. The three scan-path lookups (lookup_rom, get_rom, get_rom_by_id) use it, so a scan falls back to the other providers. get_matched_roms_by_name deliberately does not, so a manual search surfaces the error.
backend/tests/adapters/services/test_screenscraper.py	New TestCredentialErrors (12 tests: each set attributed to the right endpoint, a scraping refusal never blamed on the account, the developer variables never named, body read before status, provider wording carried through, credentials masked, no retry, a refusal on the retry attempt, later requests short-circuiting, logged once, cleared by the next scan) and TestApiClosedForAccount (401 carries both halves). Three new priming tests: an empty result, rejected credentials, and the account check never taking the provider out. Two pre-existing login-error tests retargeted to the new status and message.
backend/tests/handler/metadata/test_ss_handler.py	New TestScreenScraperCredentialFallback pins the fallback at each of the three scan-path call sites.

Testing

• Full backend suite green, run serially: 2750 passed, 2 skipped.

• trunk fmt && trunk check clean on all four files.

• A real scan per credential permutation, against the live API on a mock platform with ScreenScraper as the only metadata source. ssuserInfos.php was probed immediately before each scan, because developer-credential enforcement flaps ([Feature] Validate ScreenScraper credentials and show quota usage on the metadata sources page #4124) and the scan result has to be read against what ScreenScraper actually did:

  credentials	account check	scan outcome
  valid dev + valid user	200	2/2 identified, quota logged
  valid dev + wrong user	403	ERROR naming SCREENSCRAPER_USER / SCREENSCRAPER_PASSWORD, 2/2 still identified, quota line shows the unauthenticated cap (1000/10000, not /100000)
  wrong dev + valid user	200, then 403 during priming	account ERROR, breaker cleared, scan proceeds; jeuInfos.php then refuses and the second ERROR names the RomM developer credentials, arming the breaker on the request that mattered
  wrong dev + wrong user	403	as above
  missing dev + valid user	200, then 403 during priming	as above

  Before the account-check rule was added, rows 3 and 5 produced zero ScreenScraper metadata on an account whose scraping worked. That is what the rule exists to prevent.

• Non-403 failures (checked with a 400) still return an empty response unchanged.

Things worth a closer look

1. The outbound status is 403, and deliberately not 401. It follows the existing SS-426 mapping (blacklisted application version): both mean ScreenScraper refused RomM itself and both need an operator change, unlike the transient failures that map to 503 or the budget ones that map to 429. A 401 would have been the obvious reading, but frontend/src/services/api/index.ts treats any 401 response as an expired RomM session: it clears romm_session and redirects to the login page. The pre-existing (unreachable) credential path used 401, so simply making it reachable would have logged users out mid-search.
2. A manual name search now fails instead of quietly returning less. endpoints/search.py gathers the providers without return_exceptions, so a raised credentials error takes the whole search with it rather than returning the IGDB/MobyGames results with ScreenScraper silently missing. That is the existing behaviour for an exhausted daily quota, a blacklisted version and an offline API, so this is consistent rather than new, and the user gets an actionable message instead of an unexplained gap. Happy to catch it in get_matched_roms_by_name instead if you would rather the other providers still answer.
3. A bad developer id logs two errors, and the first one blames the account. ssuserInfos.php refuses with the account wording whichever set is actually wrong, so the account check cannot tell them apart; the scraping request that follows can, and does. Both lines are in the permutation runs above. Suppressing the first would mean staying silent about a check that genuinely failed, and suppressing the second would lose the accurate attribution, so both stay.
4. The 403 branch in _handle_client_error() is a fallback, not the main path. _attempt() converts a 403 while it still has the body, so it produces the better message. The branch in the ladder catches a 403 arriving any other way, so the hole this PR closes cannot reopen if the body read ever moves.
5. The breaker is process-wide until the next scan resets it. Since the credentials are Final constants read from the environment at import, a restart is genuinely what clears it, and a manual search hitting the short circuit still raises the full message rather than failing silently.
6. The body is now read on every request, including failures. No added cost on the success path (the previous code already called res.text() before res.json(), and aiohttp caches the body between them); error bodies are a single short line.
7. _error_message() masks credential-shaped substrings with the same SENSITIVE_KEYS_REGEX the log formatter uses. The provider's text now reaches the API response as well as the log, and the credentials travel in the query string, so it should not be echoed back verbatim on trust.

AI assistance disclosure

This change was written with AI assistance (Claude Code). The AI wrote the tests first, then the implementation, ran the full backend suite, and ran the live permutation scans reported above. I directed the design decisions (the outbound status code, attributing the refusal to a credential set without naming the developer variables, and requiring that the account check never disable the provider) and reviewed the diff.

Checklist

• I've tested the changes locally
• I've updated relevant comments
• I've assigned reviewers for this PR
• I've added unit tests that cover the changes

[greptile-apps]

Greptile Summary

ScreenScraper credential refusals are now converted into actionable, redacted errors and a breaker, while scan lookups fall back to other metadata providers. The request/retry error handling is consolidated, account-limit priming reports empty results, and tests cover credential attribution, breaker behavior, and fallback paths.

• Adds credential-set classification and clearer 403 reporting.
• Consolidates request attempts and HTTP client-error handling.
• Extends scan fallback behavior to rejected credentials.
• Adds adapter and handler regression coverage.

Confidence Score: 4/5

The process-global credential breaker needs isolation or synchronization before merging because overlapping ScreenScraper operations can clear or inherit each other's failure state.

Credential rejection state is shared by every caller while DEV_MODE permits overlapping in-process scans and manual searches, allowing one operation to short-circuit or re-enable another operation's requests.

Files Needing Attention: backend/adapters/services/screenscraper.py, backend/handler/metadata/ss_handler.py

Important Files Changed

Filename	Overview
backend/adapters/services/screenscraper.py	Adds credential rejection handling and consolidated retries, but the new breaker can interfere across overlapping process-local operations.
backend/handler/metadata/ss_handler.py	Treats credential rejection like quota exhaustion at the three scan fallback points; its scan initialization also resets the shared adapter state.
backend/tests/adapters/services/test_screenscraper.py	Thoroughly covers response classification, redaction, retries, priming, and sequential breaker behavior, but not overlapping operations.
backend/tests/handler/metadata/test_ss_handler.py	Covers credential fallback for each changed handler call site.

Prompt To Fix All With AI

### Issue 1
backend/adapters/services/screenscraper.py:652-653
**Credential breaker leaks across operations**

If ScreenScraper operations overlap in DEV_MODE, the process-global `credentials_rejected` value from one operation short-circuits another operation, while a new scan can reset or clear the first operation's breaker. This causes manual searches to fail with another scan's credential error or causes a scan to resume requests using credentials already rejected by the provider.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (1): Last reviewed commit: "fix(screenscraper): report rejected cred..." | Re-trigger Greptile}

[Spinnich]

Not changing this here @greptileai, for a few reasons.

In production the two operations named can't share the state: scans are enqueued to RQ and run in the worker process (start_bin_rq_worker), manual searches run in the gunicorn web process, and _ScanState is a module global. The overlap only exists under DEV_MODE, where scan_handler calls scan_platforms inline rather than enqueueing.

Within one process the sharing is correct rather than a leak. The credentials are process-wide Final constants read from the environment at import (config/__init__.py), so two operations cannot be using different ones; a refusal observed by either is accurate for the other. A manual search inheriting the breaker raises the accurate message instead of issuing a request ScreenScraper has already refused, which is what the breaker is for. The reverse direction costs one request before it re-arms, and re-checking at the start of a scan is intentional.

This also isn't introduced here: daily_quota_exhausted lives in the same _ScanState with identical process-global semantics. The breaker lifecycle across processes is #4123 (item 4 is exactly "in the web process the flag is never cleared"), and a fix there should cover both breakers together rather than one of them in this PR.

[greptile-apps]

That's a clear and convincing breakdown — the process boundary in production means the overlap I was worried about doesn't exist where it matters, and the DEV_MODE case sharing state inline is correct rather than a leak since the credentials are the same Final constants either way.

The point about daily_quota_exhausted having identical semantics is well-taken too; there's nothing new here, and bundling both into a proper fix in #4123 is the right scope.

Happy to drop this, and the explanation is worth a brief inline comment if you want future readers to land there without having to trace the process model themselves — but that's entirely optional.