From 503b00ce7e82ebf0061828a91082005564b5a2d9 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 11:49:46 -0400 Subject: [PATCH 1/3] Fall back to the committed OBR workbooks when obr.uk answers 200 with a non-workbook page obr.uk can answer the EFO URLs with HTTP 200 and an HTML "No Access" page instead of the xlsx. openpyxl then raised zipfile.BadZipFile, which is not a requests.RequestException, so it escaped _download_workbook before the committed-workbook fallback ran, and get_targets() dropped the nine receipts and NICs targets (34 -> 25). A body that openpyxl cannot read (BadZipFile, InvalidFileException, KeyError, OSError, ValueError) is now a permanent download failure: it is not retried, control reaches the fallback, and the warning names the URL, the status, the Content-Type and the parse error. Tests run every failure kind (200 HTML/empty/truncated zip/zip without xlsx manifest/zip without workbook part, 403, 404, 500 until retries run out, connection error) against both EFO URLs and get_targets(), with a positive control that a real workbook body is used without the fallback. Co-Authored-By: Claude Opus 5.5 --- .../obr-fallback-non-workbook.fixed.md | 1 + policyengine_uk_data/targets/sources/obr.py | 31 +++- .../tests/test_obr_efo_fallback.py | 143 +++++++++++++++++- 3 files changed, 170 insertions(+), 5 deletions(-) create mode 100644 changelog.d/obr-fallback-non-workbook.fixed.md 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..5e3e78c6 --- /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 page that is not a workbook (such as its HTML "No Access" page), not only when the download fails with an HTTP error. The parse error used to escape the fallback, and `get_targets()` silently dropped the nine OBR receipts and NICs targets. A body that is not a readable xlsx 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..69e75c22 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -12,10 +12,12 @@ import io import logging import time +import zipfile from functools import lru_cache import openpyxl import requests +from openpyxl.utils.exceptions import InvalidFileException from policyengine_uk_data.targets.schema import Target, Unit from policyengine_uk_data.targets.sources._common import ( @@ -44,6 +46,20 @@ _DOWNLOAD_MAX_ATTEMPTS = 4 _DOWNLOAD_RETRY_STATUSES = {429, 500, 502, 503, 504} +# obr.uk can also answer 200 with an HTML "No Access" page instead of the +# workbook (observed 2026-10-04 from a local machine). These are the errors +# openpyxl raises on a body that is not a readable xlsx: not a zip +# (BadZipFile), a zip without the xlsx manifest (KeyError) or workbook part +# (OSError), or malformed XML inside (ValueError). Such a body will not turn +# into a workbook on retry, so it is treated as permanent, like a 403. +_WORKBOOK_PARSE_ERRORS = ( + zipfile.BadZipFile, + InvalidFileException, + KeyError, + OSError, + ValueError, +) + # obr.uk serves 403 Forbidden to GitHub Actions runner IPs (observed on the # 2026-07-21 push builds: every attempt 403'd while the same URLs work from @@ -78,7 +94,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 +106,17 @@ 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) + try: + return openpyxl.load_workbook( + io.BytesIO(r.content), data_only=False + ) + except _WORKBOOK_PARSE_ERRORS as e: + last_error = ValueError( + f"{r.status_code} for url: {url}, but the body " + f"({r.headers.get('Content-Type', 'no Content-Type')}) " + f"is not an xlsx workbook ({type(e).__name__}: {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..ff6cace6 100644 --- a/policyengine_uk_data/tests/test_obr_efo_fallback.py +++ b/policyengine_uk_data/tests/test_obr_efo_fallback.py @@ -2,11 +2,17 @@ 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 zipfile +from contextlib import contextmanager from types import SimpleNamespace from unittest.mock import patch @@ -15,6 +21,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) @@ -28,6 +35,102 @@ 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, + ), + "403": (lambda: _response(403), 1), + "404": (lambda: _response(404), 1), + "500-until-retries-run-out": ( + lambda: _response(500), + obr._DOWNLOAD_MAX_ATTEMPTS, + ), + "connection-error": (_connection_error, obr._DOWNLOAD_MAX_ATTEMPTS), +} + +_EFO_URL_KEYS = ["efo_receipts", "efo_expenditure"] + +_RECEIPTS_AND_NICS_TARGETS = { + "obr/income_tax", + "obr/ni", + "obr/vat", + "obr/fuel_duties", + "obr/capital_gains_tax", + "obr/sdlt", + "obr/ni_employee", + "obr/ni_employer", + "obr/ni_self_employed", +} + + +@contextmanager +def _obr_answering(make_response): + """Answer every requests.get with make_response(), skip retry sleeps and + spy on the fallback. Yields the request and fallback call logs.""" + requests_made, fallbacks_used = [], [] + real_fallback = obr._fallback_workbook + + def get(url, *args, **kwargs): + requests_made.append(url) + return make_response() + + def fallback(url): + wb = real_fallback(url) + fallbacks_used.append((url, wb)) + return wb + + with ( + patch.object(obr.requests, "get", side_effect=get), + patch.object(obr.time, "sleep", lambda s: None), + patch.object(obr, "_fallback_workbook", side_effect=fallback), + ): + yield requests_made, fallbacks_used + + def test_fallback_workbooks_are_committed_and_parseable(): for filename in obr._EFO_FALLBACKS.values(): path = STORAGE_FOLDER / "obr_efo" / filename @@ -94,3 +197,37 @@ def get(*args, **kwargs): "obr/vat", } <= names assert len(names) >= 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 From cc7550101d289fda4d9cde3f92fa68e78982820f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 12:26:38 -0400 Subject: [PATCH 2/3] Catch every openpyxl failure on a non-workbook 200 body, per review A zip with malformed XML inside raised xml.etree.ElementTree.ParseError (a SyntaxError), which escaped the named-exception tuple; zlib.error, NotImplementedError, RuntimeError and TypeError could too. Catch Exception around the single load_workbook call on the response body: a genuine openpyxl break still surfaces, because the fallback is parsed by the same call outside any try. Chain the parse error as the cause of the reported ValueError. Tests: add a malformed-XML row; pin the fallback warning text and the ValueError raised for a URL with no committed workbook; fold the 403 and connection-error tests into the response-kind table they duplicated. Co-Authored-By: Claude Opus 5.5 --- .../obr-fallback-non-workbook.fixed.md | 2 +- policyengine_uk_data/targets/sources/obr.py | 24 +++---- .../tests/test_obr_efo_fallback.py | 62 ++++++++----------- 3 files changed, 33 insertions(+), 55 deletions(-) diff --git a/changelog.d/obr-fallback-non-workbook.fixed.md b/changelog.d/obr-fallback-non-workbook.fixed.md index 5e3e78c6..c36e8642 100644 --- a/changelog.d/obr-fallback-non-workbook.fixed.md +++ b/changelog.d/obr-fallback-non-workbook.fixed.md @@ -1 +1 @@ -Fall back to the committed OBR EFO workbooks when obr.uk answers 200 with a page that is not a workbook (such as its HTML "No Access" page), not only when the download fails with an HTTP error. The parse error used to escape the fallback, and `get_targets()` silently dropped the nine OBR receipts and NICs targets. A body that is not a readable xlsx is now treated as a permanent failure: it is not retried, and a warning names the URL, the status and the parse error. +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 the nine OBR receipts and NICs targets, 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 69e75c22..5a1286b7 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -12,12 +12,10 @@ import io import logging import time -import zipfile from functools import lru_cache import openpyxl import requests -from openpyxl.utils.exceptions import InvalidFileException from policyengine_uk_data.targets.schema import Target, Unit from policyengine_uk_data.targets.sources._common import ( @@ -46,20 +44,6 @@ _DOWNLOAD_MAX_ATTEMPTS = 4 _DOWNLOAD_RETRY_STATUSES = {429, 500, 502, 503, 504} -# obr.uk can also answer 200 with an HTML "No Access" page instead of the -# workbook (observed 2026-10-04 from a local machine). These are the errors -# openpyxl raises on a body that is not a readable xlsx: not a zip -# (BadZipFile), a zip without the xlsx manifest (KeyError) or workbook part -# (OSError), or malformed XML inside (ValueError). Such a body will not turn -# into a workbook on retry, so it is treated as permanent, like a 403. -_WORKBOOK_PARSE_ERRORS = ( - zipfile.BadZipFile, - InvalidFileException, - KeyError, - OSError, - ValueError, -) - # obr.uk serves 403 Forbidden to GitHub Actions runner IPs (observed on the # 2026-07-21 push builds: every attempt 403'd while the same URLs work from @@ -106,16 +90,22 @@ def _download_workbook(url: str) -> openpyxl.Workbook: last_error = e # connection/timeout — retryable else: if r.status_code < 400: + # 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. Catching broadly cannot hide an openpyxl break: + # the fallback is parsed by the same call outside any try. try: return openpyxl.load_workbook( io.BytesIO(r.content), data_only=False ) - except _WORKBOOK_PARSE_ERRORS as e: + 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')}) " f"is not an xlsx workbook ({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 ff6cace6..ecc25c9a 100644 --- a/policyengine_uk_data/tests/test_obr_efo_fallback.py +++ b/policyengine_uk_data/tests/test_obr_efo_fallback.py @@ -11,6 +11,7 @@ """ import io +import logging import zipfile from contextlib import contextmanager from types import SimpleNamespace @@ -31,10 +32,6 @@ 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) @@ -83,6 +80,10 @@ def _connection_error(): ), 1, ), + "200-zip-with-malformed-xml": ( + lambda: _response(200, _zip({"[Content_Types].xml": " Date: Sun, 4 Oct 2026 12:50:01 -0400 Subject: [PATCH 3/3] Tighten the fallback comment, message and changelog; pin cause chaining Second review round: the broad catch only guarantees that an openpyxl break on every workbook still surfaces, so the comment now says that; the message says the body "could not be read as an xlsx workbook", which also covers a served file openpyxl rejects. The changelog says the defect drops every target from the affected workbook (nine in the observed receipts case), and the no-fallback test asserts the parse error is chained as the cause. Co-Authored-By: Claude Opus 5.5 --- changelog.d/obr-fallback-non-workbook.fixed.md | 2 +- policyengine_uk_data/targets/sources/obr.py | 7 ++++--- .../tests/test_obr_efo_fallback.py | 17 +++++++++++------ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/changelog.d/obr-fallback-non-workbook.fixed.md b/changelog.d/obr-fallback-non-workbook.fixed.md index c36e8642..4ec77488 100644 --- a/changelog.d/obr-fallback-non-workbook.fixed.md +++ b/changelog.d/obr-fallback-non-workbook.fixed.md @@ -1 +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 the nine OBR receipts and NICs targets, 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. +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 5a1286b7..8a87c56c 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -93,8 +93,8 @@ def _download_workbook(url: str) -> openpyxl.Workbook: # 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. Catching broadly cannot hide an openpyxl break: - # the fallback is parsed by the same call outside any try. + # 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 @@ -103,7 +103,8 @@ def _download_workbook(url: str) -> openpyxl.Workbook: last_error = ValueError( f"{r.status_code} for url: {url}, but the body " f"({r.headers.get('Content-Type', 'no Content-Type')}) " - f"is not an xlsx workbook ({type(e).__name__}: {e})" + "could not be read as an xlsx workbook " + f"({type(e).__name__}: {e})" ) last_error.__cause__ = e break diff --git a/policyengine_uk_data/tests/test_obr_efo_fallback.py b/policyengine_uk_data/tests/test_obr_efo_fallback.py index ecc25c9a..07169a88 100644 --- a/policyengine_uk_data/tests/test_obr_efo_fallback.py +++ b/policyengine_uk_data/tests/test_obr_efo_fallback.py @@ -143,13 +143,18 @@ def test_fallback_workbooks_are_committed_and_parseable(): @pytest.mark.parametrize( - "kind, error", [("403", requests.HTTPError), ("200-html-page", ValueError)] + "kind, error, cause", + [ + ("403", requests.HTTPError, type(None)), + ("200-html-page", ValueError, zipfile.BadZipFile), + ], ) -def test_unknown_url_with_failed_download_still_raises(kind, error): +def test_unknown_url_with_failed_download_still_raises(kind, error, cause): url = "https://obr.uk/download/some-other-file/" with _obr_answering(_FAILED_DOWNLOADS[kind][0]): - with pytest.raises(error, match=f"for url: {url}"): + with pytest.raises(error, match=f"for url: {url}") as raised: obr._download_workbook(url) + assert isinstance(raised.value.__cause__, cause) def test_full_target_set_available_offline(): @@ -215,7 +220,7 @@ def test_non_workbook_200_warning_names_url_status_and_parse_error(caplog): 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) is not " - "an xlsx workbook (BadZipFile: File is not a zip file)); using " - "committed workbook fallback" + 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