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"No Access - Office for Budget Responsibility" + b"

No Access

" +) + +# Every way the download can fail, with the number of requests it should +# cost: transient failures use the full retry budget, permanent ones one. +_FAILED_DOWNLOADS = { + "200-html-page": ( + lambda: _response(200, _NO_ACCESS_PAGE, "text/html; charset=UTF-8"), + 1, + ), + "200-empty-body": (lambda: _response(200), 1), + "200-truncated-zip": (lambda: _response(200, b"PK\x03\x04garbage"), 1), + "200-zip-without-xlsx-manifest": ( + lambda: _response(200, _zip({"readme.txt": "not a workbook"})), + 1, + ), + "200-zip-without-workbook-part": ( + lambda: _response( + 200, + _zip( + { + "[Content_Types].xml": '' + } + ), + ), + 1, + ), + "200-zip-with-malformed-xml": ( + lambda: _response(200, _zip({"[Content_Types].xml": "= 30 + + +@pytest.mark.parametrize("url_key", _EFO_URL_KEYS) +@pytest.mark.parametrize("kind", list(_FAILED_DOWNLOADS)) +def test_every_failed_download_uses_committed_workbook(kind, url_key): + make_response, expected_requests = _FAILED_DOWNLOADS[kind] + url = load_config()["obr"][url_key] + with _obr_answering(make_response) as (requests_made, fallbacks_used): + wb = obr._download_workbook(url) + assert fallbacks_used == [(url, wb)], "the committed workbook was not returned" + assert len(requests_made) == expected_requests + + +@pytest.mark.parametrize("kind", list(_FAILED_DOWNLOADS)) +def test_every_failed_download_keeps_receipts_and_nics_targets(kind): + make_response, _ = _FAILED_DOWNLOADS[kind] + with _obr_answering(make_response): + names = {t.name for t in obr.get_targets()} + assert _RECEIPTS_AND_NICS_TARGETS <= names, sorted( + _RECEIPTS_AND_NICS_TARGETS - names + ) + + +def test_workbook_response_is_parsed_without_fallback(): + """Positive control: a real xlsx body is used as served.""" + body = (STORAGE_FOLDER / "obr_efo" / "efo_receipts.xlsx").read_bytes() + with _obr_answering(lambda: _response(200, body)) as ( + requests_made, + fallbacks_used, + ): + wb = obr._download_workbook(load_config()["obr"]["efo_receipts"]) + assert fallbacks_used == [] + assert len(requests_made) == 1 + assert obr._find_receipts_sheet(wb) is not None + + +def test_non_workbook_200_warning_names_url_status_and_parse_error(caplog): + caplog.set_level(logging.WARNING, logger=obr.logger.name) + url = load_config()["obr"]["efo_receipts"] + with _obr_answering(_FAILED_DOWNLOADS["200-html-page"][0]): + obr._download_workbook(url) + assert ( + f"200 for url: {url}, but the body (text/html; charset=UTF-8) could " + "not be read as an xlsx workbook (BadZipFile: File is not a zip " + "file)); using committed workbook fallback" + ) in caplog.text