From ea09fd0a84f357d3fdc67fb24d68925f24439be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Marcoux?= Date: Wed, 3 Jun 2026 09:05:09 +0200 Subject: [PATCH] fix(timeshift): keep cascading on upstream 5xx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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". --- apps/timeshift/tests/test_views.py | 23 +++++++++++++++++++++-- apps/timeshift/views.py | 12 ++++++++++-- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/apps/timeshift/tests/test_views.py b/apps/timeshift/tests/test_views.py index 857effab..e443547d 100644 --- a/apps/timeshift/tests/test_views.py +++ b/apps/timeshift/tests/test_views.py @@ -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): diff --git a/apps/timeshift/views.py b/apps/timeshift/views.py index cd838fd1..f265783f 100644 --- a/apps/timeshift/views.py +++ b/apps/timeshift/views.py @@ -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: