diff --git a/changelog.d/obr-fallback-non-workbook.fixed.md b/changelog.d/obr-fallback-non-workbook.fixed.md new file mode 100644 index 00000000..4ec77488 --- /dev/null +++ b/changelog.d/obr-fallback-non-workbook.fixed.md @@ -0,0 +1 @@ +Fall back to the committed OBR EFO workbooks when obr.uk answers 200 with a body that is not a workbook (such as its HTML "No Access" page), not only when the request fails. The parse error used to escape the fallback, and `get_targets()` dropped every target parsed from that workbook (the nine OBR receipts and NICs targets in the case observed), logging the error while the build carried on. A body openpyxl cannot read is now treated as a permanent failure: it is not retried, and a warning names the URL, the status and the parse error. diff --git a/policyengine_uk_data/targets/sources/obr.py b/policyengine_uk_data/targets/sources/obr.py index ad4e3cf1..8a87c56c 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -78,7 +78,8 @@ def _download_workbook(url: str) -> openpyxl.Workbook: Retries transient HTTP errors (429/5xx) and connection failures with exponential backoff, honouring a numeric Retry-After header when present. Falls back to the committed workbook in storage/obr_efo/ when - the download ultimately fails (obr.uk 403s CI runner IPs). + the download ultimately fails: obr.uk 403s CI runner IPs, and can answer + 200 with an HTML page that is not a workbook. Neither is retried. """ last_error: Exception | None = None for attempt in range(_DOWNLOAD_MAX_ATTEMPTS): @@ -89,7 +90,24 @@ def _download_workbook(url: str) -> openpyxl.Workbook: last_error = e # connection/timeout — retryable else: if r.status_code < 400: - return openpyxl.load_workbook(io.BytesIO(r.content), data_only=False) + # obr.uk can answer 200 with an HTML "No Access" page instead + # of the workbook (observed 2026-10-04). A body openpyxl cannot + # read will not become a workbook on retry, so it is permanent, + # like a 403. An openpyxl break on every workbook still + # surfaces, as the fallback goes through the same load_workbook. + try: + return openpyxl.load_workbook( + io.BytesIO(r.content), data_only=False + ) + except Exception as e: + last_error = ValueError( + f"{r.status_code} for url: {url}, but the body " + f"({r.headers.get('Content-Type', 'no Content-Type')}) " + "could not be read as an xlsx workbook " + f"({type(e).__name__}: {e})" + ) + last_error.__cause__ = e + break last_error = requests.HTTPError( f"{r.status_code} for url: {url}", response=r ) diff --git a/policyengine_uk_data/tests/test_obr_efo_fallback.py b/policyengine_uk_data/tests/test_obr_efo_fallback.py index 309e8d7c..07169a88 100644 --- a/policyengine_uk_data/tests/test_obr_efo_fallback.py +++ b/policyengine_uk_data/tests/test_obr_efo_fallback.py @@ -2,11 +2,18 @@ obr.uk serves 403 Forbidden to GitHub Actions runner IPs, and losing the workbooks silently drops 28 OBR targets and calibrates a degraded dataset -(observed on the 2026-07-21 push builds). These tests pin: the fallback -workbooks are committed and parseable, a failed download uses them instead -of raising, and a 403 does not burn the retry budget. +(observed on the 2026-07-21 push builds). obr.uk can also answer 200 with +an HTML "No Access" page, which once escaped the fallback as a BadZipFile +and dropped the nine receipts and NICs targets (observed 2026-10-04). These +tests pin: the fallback workbooks are committed and parseable, every kind of +failed download uses them instead of raising, and permanent failures do not +burn the retry budget. """ +import io +import logging +import zipfile +from contextlib import contextmanager from types import SimpleNamespace from unittest.mock import patch @@ -15,6 +22,7 @@ from policyengine_uk_data.storage import STORAGE_FOLDER from policyengine_uk_data.targets.sources import obr +from policyengine_uk_data.targets.sources._common import load_config @pytest.fixture(autouse=True) @@ -24,8 +32,104 @@ def _clear_workbook_cache(): obr._download_workbook.cache_clear() -def _forbidden(*args, **kwargs): - return SimpleNamespace(status_code=403, headers={}, content=b"") +def _response(status_code, content=b"", content_type=None): + headers = {"Content-Type": content_type} if content_type else {} + return SimpleNamespace(status_code=status_code, headers=headers, content=content) + + +def _zip(members: dict[str, str]) -> bytes: + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + for name, data in members.items(): + archive.writestr(name, data) + return buffer.getvalue() + + +def _connection_error(): + raise requests.ConnectionError("no route to obr.uk") + + +_NO_ACCESS_PAGE = ( + b"
" + b"