fix(timeshift): keep cascading on upstream 5xx

Some XC servers run PHP with display_errors off, so a timestamp shape
their parser rejects comes back as a hard HTTP 500 instead of a 200 with
inline PHP warning text. The candidate-format cascade treated any 5xx as
decisive and stopped, so catch-up never reached the timestamp shape that
works on those servers and failed outright on them.

Only short-circuit on genuinely decisive, ban-sensitive statuses
(401/403/406 and 3xx redirects — a 302 is the first sign of an XC ban);
treat 5xx like 400/404 and try the next candidate shape. Add regression
tests for "500 then success" and "all candidates 500".
This commit is contained in:
Cédric Marcoux 2026-06-03 09:05:09 +02:00
parent e154e4e696
commit ea09fd0a84
2 changed files with 31 additions and 4 deletions

View file

@ -98,11 +98,30 @@ class StreamFromProviderStatusMappingTests(TestCase):
self.assertEqual(mocked_open.call_count, 1)
@patch.object(views, "_open_upstream")
def test_upstream_500_short_circuits_loop(self, mocked_open):
def test_upstream_500_continues_to_next_candidate(self, mocked_open):
# A 5xx is format-specific on many XC servers (PHP fatal with
# display_errors off turns an "Undefined array key" warning into a
# hard 500), so the cascade must keep trying — the next timestamp
# shape often succeeds. Regression: providers that 500 on the first
# shape used to fail outright because the loop short-circuited.
mocked_open.side_effect = [
_fake_upstream(500),
_fake_upstream(200, body=_make_ts_payload()),
]
with patch.object(views, "RedisClient"), \
patch.object(views, "_register_stats_client"), \
patch.object(views, "_unregister_stats_client"):
response = views._stream_from_provider(**self.kwargs)
self.assertEqual(response.status_code, 200)
self.assertEqual(mocked_open.call_count, 2)
@patch.object(views, "_open_upstream")
def test_all_candidates_500_returns_error(self, mocked_open):
# Every shape 500s → all candidates attempted, then a clean error.
mocked_open.return_value = _fake_upstream(500)
response = views._stream_from_provider(**self.kwargs)
self.assertEqual(response.status_code, 400)
self.assertEqual(mocked_open.call_count, 1)
self.assertEqual(mocked_open.call_count, 3)
@patch.object(views, "_open_upstream")
def test_first_candidate_succeeds(self, mocked_open):

View file

@ -434,8 +434,16 @@ def _stream_from_provider(
last_status = 404 # Treat as soft rejection for cascade
continue
response.close()
if response.status_code not in (400, 404):
# Decisive failure (403, 5xx, …) — no point trying alternative URLs.
# Decisive statuses where trying other URL shapes can't help and may
# escalate an IP ban: auth failures (401/403), IP-level block (406), and
# any redirect (3xx) — for XC providers a 302 is the first sign of a ban.
# A 5xx is different: it is usually format-specific — some XC servers run
# PHP with display_errors off, so the "Undefined array key" warning that
# another server emits as 200+text becomes a hard 500. The very next
# candidate timestamp shape often succeeds, so keep trying instead of
# giving up (this is why catch-up appeared broken on providers that 500).
code = response.status_code
if code in (401, 403, 406) or 300 <= code < 400:
break
if winning_index is not None: