diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index e98b6d2..15cab32 100644 --- a/docs/CICs/UNFAOPostProcessorManager.md +++ b/docs/CICs/UNFAOPostProcessorManager.md @@ -3,7 +3,7 @@ **Status:** Active **Owner:** PRIO MD&D Team -**Last reviewed:** 2026-08-05 +**Last reviewed:** 2026-08-17 **Related ADRs:** ADR-001, ADR-002, ADR-008, ADR-009 --- @@ -93,7 +93,7 @@ Assumptions that are not met **must cause failure**, not fallback behavior. The - **Wrong forecast selected:** structurally impossible since #149. Selection is by **run manifest** — a commit marker whose contents are hash-verified — not by scanning the bucket for the newest `category="forecast"` upload. Declared identity is additionally checked **per shard header** against the launched ensemble inside `TargetLease.load()` (`contract/wire/source_selection.py:73-81`), so identity comes from the artifact's own content. The metadata-field check this bullet used to describe (`delivery/identity.py`) was retired in #150 and the legacy reader it served in #149; register C-25 is closed as *superseded by mechanism* - **Launch config incomplete:** raises `LaunchConfigError` naming the missing key. A launcher that omits `wire_contract` or declares a `data_format` other than `feature_frame` is **refused**, never quietly routed into a fallback (ADR-003, register C-63) - **Region coverage mismatch:** Raises `CoverageError` in `_check_coverage()` (called from `_validate()`) if a pinned region's delivered cell count is wrong (S1/C-34) or a GAUL-uncovered excluded cell leaks into the delivery (S4/C-30) -- **Fabricated historical tail:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) so unobserved zero-padding is not shipped as observed history (S2/C-26); **degrades open** (skips the clip with a WARNING) if the boundary cannot be resolved +- **Fabricated historical tail:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) so unobserved zero-padding is not shipped as observed history (S2/C-26). Two outcomes when the boundary is unavailable, and they are different on purpose (C-103, 2026-08-17): if the producer simply publishes no boundary — or the read fails — it **degrades open**, skipping the clip with a WARNING that states the unobserved tail will ship; if the producer client cannot be imported at all it **refuses** (`source_metadata.ProducerClientMissing`), because a broken environment is not a producer fact - **Upload provenance:** the historical artifact's `description` carries structured provenance (lookup version, region, expected/actual cell counts, unmapped count) built by `delivery/provenance.py` (`build_provenance` → `compact_description`) via the manager's `_historical_frame_description()` (S5/C-15). The **forecast** side carries no such description: its guarantee is the wire's verified chain — per-shard content hashes recorded in the §4.2 run manifest, header asserts on load, and manifest-last commit ordering. That is identity and integrity, not the C-15 provenance field set; the §4.2 manifest's keys are exactly `contract_version`, `run_id`, `targets`, `shards`, `expected_months`, `expected_cell_count`, `sidecar` — and it carries **no** `lookup_version`, `region` or `unmapped_count`. `_delivery_description()` was the pandas-path equivalent and was deleted with it in #149 The following **must never** fail silently: diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 571a59e..0cf8b12 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -1064,15 +1064,24 @@ Recorded rather than left implicit because "the operator did a console session" | Field | Value | |-------|-------| | ID | C-103 | -| Tier | 2 — structural fragility with a named scenario: the guard's dependency is absent, the absence is caught by a bare `except Exception`, and the delivery proceeds unclipped. Held at 2 rather than 1 because of a gap, not a judgement — see the verification question below. | +| Tier | **3** — re-tiered down from 2 on 2026-08-17, when the verification question below was answered and the premise did not hold. The residual is real but narrower: not "the dependency is missing", which a different guard already refuses, but "any failure to read the boundary degrades open". | | Source | `/repo-assimilation` (2026-08-16), measured | -| Trigger | When the launcher's environment is next built or changed — a views-models deploy, a container rebuild, a `poetry install` on the delivery host — verify `import datafactory_query` succeeds there. The delivery will not tell you if it does not. | +| Trigger | When a delivery logs *"last_valid_month_id could not be read; skipping the observed-range clip"* — that is now the only route to an unclipped delivery, and it is a real one (a network failure or a reshaped `.zattrs` reaches it). Decide then whether degrade-open is still the right side for that case, or whether the partner should be told the tail is unverified. | | Owner | This repository, for the swallow and the declaration. The producer owns the fact itself. | -| Location | `views_postprocessing/contract/source_metadata.py:37`; `views_postprocessing/unfao/managers/unfao.py:137-142`, `views_postprocessing/crafd/managers/crafd.py:137-142`; `pyproject.toml` (the dependency is absent) | +| Location | `views_postprocessing/contract/source_metadata.py:74` (the classification); `views_postprocessing/unfao/managers/unfao.py:137-151`, `views_postprocessing/crafd/managers/crafd.py:137-151` (the two branches). **Not `pyproject.toml`** — the original entry listed it as a risk site on the assumption the dependency was undeclared everywhere; it is the launcher's to declare and both launchers do, so there is nothing to add here. Line numbers re-read 2026-08-17 after the fix moved them. | `source_metadata.last_valid_month_id` lazily imports `datafactory_query.defaults`. Measured 2026-08-16: that package is in neither `pyproject.toml` nor `poetry.lock`, and `import datafactory_query` raises `ModuleNotFoundError` in the project venv. Its only caller wraps the call in `except Exception: lv = None` and then returns the historical frame **unclipped**, logging one WARNING — so "the dependency is missing" and "the producer publishes no boundary attribute" leave through the same branch with the same outcome, and that outcome is unobserved zero-padded months shipping to the partner as observed history. The lazy import states its own reason — *"so this module loads without the heavy datafactory dependency present (e.g. in unit-test environments)"* — but nothing at the call site distinguishes a unit-test environment from a delivery. -**Verification question, stated as a gap rather than dressed as a finding.** Whether the production launcher's environment supplies `datafactory_query` could not be established from this repository; views-models builds that environment. If it does, this is a declaration gap and the swallow is the whole risk. If it does not, C-26's fabrication has been shipping unclipped since #126. **Do not downgrade on inspection of this repo alone** — the instruction C-26 already carries, for the same reason. +**ANSWERED 2026-08-17, and the answer moves the tier.** The question was whether the production launcher supplies `datafactory_query`. It does, twice over: + +- views-models `postprocessors/un_fao/requirements.txt` and `postprocessors/un_crafd/requirements.txt` both pin **`views-datafactory>=1.9.0,<2.0.0`**, which is what ships the `datafactory_query` module (there is no separate distribution — `pip download datafactory-query` finds nothing; it is one of nine packages in views-datafactory's wheel). +- More decisively, the postprocessor's `config_queryset.py` imports `datafactory_query.defaults` **at module scope** and raises a `RuntimeError` naming the fix if it is absent. A missing client therefore makes `get_queryset()` return `None`, which `launch_config.assert_queryset_was_importable` turns into a refusal (**C-83**) *before* `_read_historical_frame` is ever reached. + +So the scenario this entry was filed on — a missing dependency silently shipping fabricated months on the live path — **cannot occur**. It was already guarded, by a check written for a different reason. The Tier 2 rested on a premise measured only in *this* repository's venv, and the instruction it borrowed from C-26 (*"do not downgrade on inspection of this repo alone"*) was the right instruction: the resolution came from reading the launcher, not from reading here. + +**What is genuinely left, and it is why this stays open at Tier 3.** The `except Exception` was never only about the import. A network failure, an auth error, a reshaped `.zattrs`, a timeout — all still leave through one branch, log one WARNING, and deliver the unobserved tail as observed history. That is the recorded C-26 degrade-open decision applied far more broadly than C-26 argued for. + +**Partial mitigation, 2026-08-17.** `source_metadata` now raises `ProducerClientMissing` instead of letting the import failure fall into the caller's broad `except`, and both managers re-raise it rather than degrading. Defence in depth for the paths `assert_queryset_was_importable` does not cover — a direct caller, a future launcher, a partner not going through the same queryset. The module also gained its first tests (`tests/test_source_metadata.py`, 7, mutation-proven against both the pre-fix return-`None` and a manager that collapses the two branches back into one); it had **none** before, which is how "return None like everything else" ever looked reasonable. The broad degrade-open is deliberately unchanged — narrowing it is a decision about what to tell the partner, not a refactor. **C-60 is this shape, and it was resolved by deleting the degradation.** There, a provenance stamp reached into the producer's ledger schema inside a bare `except … pass` and returned `"unknown"`; the fix was to raise. The difference here is that the degradation is deliberate and documented ("degrade-open, C-26") — which makes the question *whether the open side is still the right one*, not whether someone forgot. diff --git a/tests/test_source_metadata.py b/tests/test_source_metadata.py new file mode 100644 index 0000000..51d29a7 --- /dev/null +++ b/tests/test_source_metadata.py @@ -0,0 +1,197 @@ +"""The producer-fact seam: what happens when the producer cannot be asked (C-103). + +``contract/source_metadata.py`` is the single place this repository asks +views-datafactory for a data fact, and until 2026-08-17 it had **no tests at all** — +which is how the defect below survived: nothing described what the module should do +when the client is absent, so "return None like everything else" looked reasonable. + +The distinction these tests pin is the whole point of the module: + +* the producer publishes **no boundary** — a normal, older store. The caller degrades + open and delivers unclipped (C-26, a recorded decision). +* the producer **cannot be asked at all** — a broken environment. Degrading open here + ships the unobserved zero-padded tail as observed history, and does it with one + WARNING in a log nobody reads. + +Same shape as C-60, where a provenance stamp degraded to ``"unknown"`` on a bare +except and made every delivery untraceable in exactly the field it existed to answer. +""" + +from __future__ import annotations + +import sys +import types +from pathlib import Path + +import pytest + +from tests.conftest import PARTNER_PACKAGES +from views_postprocessing.contract import source_metadata + +_REPO = Path(__file__).resolve().parent.parent + +#: The numpy 1.x/2.x ABI break recorded in views-models +#: `postprocessors/un_fao/requirements.txt` on 2026-08-13, found by the pre-delivery +#: rehearsal. It is a `ValueError`, not an `ImportError` — which is why the guard +#: catches `Exception`. +_ABI_BREAK = ( + "numpy.dtype size changed, may indicate binary incompatibility. " + "Expected 96 from C header, got 88" +) + + +@pytest.fixture +def absent_client(monkeypatch): + """Make `datafactory_query` unimportable REGARDLESS of what is installed. + + The first draft of this module relied on the package happening to be absent in the + developer venv, which meant four of these tests would have failed in the launcher + prefix — the environment they describe. That is the C-30/C-46 shape again: a guard + whose proof rests on an ambient property nothing declares. + """ + monkeypatch.setitem(sys.modules, "datafactory_query", None) + monkeypatch.setitem(sys.modules, "datafactory_query.defaults", None) + + +@pytest.fixture +def exploding_client(monkeypatch): + """`datafactory_query` is INSTALLED and raises on load — the real-world case.""" + + class _Exploding: + __name__ = "datafactory_query.defaults" + + def __getattr__(self, name): + raise ValueError(_ABI_BREAK) + + module = types.ModuleType("datafactory_query") + defaults = _Exploding() + module.defaults = defaults + monkeypatch.setitem(sys.modules, "datafactory_query", module) + monkeypatch.setitem(sys.modules, "datafactory_query.defaults", defaults) + + +def test_an_absent_client_raises_rather_than_reporting_no_boundary(absent_client): + with pytest.raises(source_metadata.ProducerClientUnavailable): + source_metadata.last_valid_month_id() + + +def test_an_absent_client_is_told_to_install_the_package(absent_client): + with pytest.raises(source_metadata.ProducerClientUnavailable) as excinfo: + source_metadata.last_valid_month_id() + message = str(excinfo.value) + assert "not installed" in message + assert "views-datafactory" in message, "the refusal must name the package to install" + # The reader must be able to tell this apart from the degrade-open case. + assert "observed" in message, ( + "the refusal must say what degrading open would have cost — unobserved months " + "shipping as observed history — or it reads as a routine unavailability" + ) + + +def test_a_client_that_raises_on_load_is_NOT_reported_as_missing(exploding_client): + """The failure this environment actually had: a ValueError, not an ImportError. + + An `except ImportError` clause would let this reach the caller's degrade-open and + ship the unobserved tail — the precise case the guard exists to separate. + """ + with pytest.raises(source_metadata.ProducerClientUnavailable) as excinfo: + source_metadata.last_valid_month_id() + message = str(excinfo.value) + assert "present but raised while loading" in message + assert "ValueError" in message and "numpy.dtype size changed" in message, ( + "the refusal must quote what actually went wrong; the operator cannot act on " + "'could not be loaded' alone" + ) + assert "Do NOT reinstall" in message, ( + "reporting an installed-but-broken package as missing sends the operator to a " + "fix they have already applied" + ) + + +def test_the_refusal_chains_the_original_exception(absent_client): + """``raise ... from exc``: the traceback must still say what actually failed.""" + with pytest.raises(source_metadata.ProducerClientUnavailable) as excinfo: + source_metadata.last_valid_month_id() + assert isinstance(excinfo.value.__cause__, ImportError) + + +def test_it_is_logged_as_well_as_raised(caplog, absent_client): + """ADR-008: a refusal on the delivery path is logged persistently AND raised.""" + with caplog.at_level("ERROR"), pytest.raises(source_metadata.ProducerClientUnavailable): + source_metadata.last_valid_month_id() + assert any(r.levelname == "ERROR" for r in caplog.records), ( + "the refusal was raised but never logged; a traceback that dies inside a " + "scheduled run leaves no persistent record (ADR-008)" + ) + + +def test_the_producers_answer_passes_through_untouched(monkeypatch): + """When the client IS present, this module reads and returns — it does not decide. + + Including ``None``: a store that predates the attribute reports no boundary, and + that is the case the caller is entitled to degrade open on. + """ + import types + + for answer in (137, None): + fake = types.ModuleType("datafactory_query") + defaults = types.ModuleType("datafactory_query.defaults") + seen = {} + + def get_last_valid_month_id(zarr_url=None, _answer=answer): + seen["zarr_url"] = zarr_url + return _answer + + defaults.get_last_valid_month_id = get_last_valid_month_id + fake.defaults = defaults + monkeypatch.setitem(sys.modules, "datafactory_query", fake) + monkeypatch.setitem(sys.modules, "datafactory_query.defaults", defaults) + + assert source_metadata.last_valid_month_id("zarr://declared") == answer + assert seen["zarr_url"] == "zarr://declared", ( + "the declared store must be passed through; guessing the producer's " + "default here would be the inference ADR-003 forbids" + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_managers_do_not_swallow_the_refusal(partner): + """The degrade-open must not re-absorb what this module just refused. + + Parametrized over ``PARTNER_PACKAGES`` rather than a literal pair: eight guards + once hardcoded ``"unfao"`` and all eight went on passing over ``crafd/`` when it + landed (see ``tests/conftest.py``). A ninth would have been this one. + + A source check rather than a behavioural one, deliberately: constructing a manager + needs pipeline-core, a path manager and an Appwrite environment (C-40), and the + property worth pinning is one line — that ``ProducerClientUnavailable`` is re-raised + *before* the broad ``except``. Someone tidying the two branches back into one is + precisely how C-103 would return, and it would return silently. + """ + source = (_REPO / "views_postprocessing" / partner / "managers" / f"{partner}.py").read_text() + + # Scope to `_read_historical_frame`. Comparing offsets across the whole file would + # let a narrow branch on some unrelated `try` satisfy the ordering while the + # boundary read's branch was gone. + start = source.index("def _read_historical_frame") + body = source[start:source.index("\n def ", start)] + + narrow = body.count("except source_metadata.ProducerClientUnavailable:") + broad = body.count("except Exception:") + assert narrow == 1, ( + f"{partner}'s _read_historical_frame has {narrow} ProducerClientUnavailable " + "branches, expected 1. Without it a broken environment is indistinguishable " + "from a producer that publishes no boundary, and the delivery ships fabricated " + "months either way (C-103)." + ) + assert broad == 1, ( + f"{partner}'s _read_historical_frame has {broad} broad `except Exception:` " + "branches, expected 1 (the C-26 degrade-open). If it is gone the refusal may " + "be fine, but this check no longer describes the code — read it and rewrite it." + ) + assert body.index("except source_metadata.ProducerClientUnavailable:") < body.index( + "except Exception:" + ), ( + f"{partner} catches Exception before ProducerClientUnavailable, so the narrow " + "branch is unreachable" + ) diff --git a/views_postprocessing/contract/source_metadata.py b/views_postprocessing/contract/source_metadata.py index 49b163a..df2ee2f 100644 --- a/views_postprocessing/contract/source_metadata.py +++ b/views_postprocessing/contract/source_metadata.py @@ -20,6 +20,16 @@ logger = logging.getLogger(__name__) +class ProducerClientUnavailable(RuntimeError): + """``datafactory_query`` could not be loaded, so no producer fact can be read. + + One type, two causes, and the message says which: the package is not installed, or + it is installed and raised while loading. Both are broken environments; neither is + a producer that publishes no boundary. Named "unavailable" rather than "missing" + because the second cause is the one this environment has actually had. + """ + + def last_valid_month_id(zarr_url: str | None = None) -> int | None: """The producer's last *observed* month for the served zarr (a datafactory fact). @@ -31,9 +41,69 @@ def last_valid_month_id(zarr_url: str | None = None) -> int | None: zarr_url: the served zarr; ``None`` uses datafactory's default store (which the FAO queryset itself uses — ``ZARR_URL = DEFAULT_REMOTE.zarr_url``). - The import is lazy so this module loads without the heavy datafactory dependency - present (e.g. in unit-test environments). + Raises: + ProducerClientUnavailable: if ``datafactory_query`` cannot be loaded — + whether because it is absent or because importing it raised. + + **The dependency is the launcher's to supply, and it does.** ``datafactory_query`` + ships inside ``views-datafactory``; it is not separately installable and it is not + declared in this repository's ``pyproject.toml``. Both launchers declare it — + views-models ``postprocessors/{un_fao,un_crafd}/requirements.txt`` pin + ``views-datafactory>=1.9.0,<2.0.0``. The import stays lazy so this module, and + everything that imports it, loads in a unit-test environment without the producer's + (heavy) package present. + + **Why this raises rather than returning None (register C-103).** The caller degrades + open when the boundary is unavailable — a deliberate C-26 decision, because a producer + that publishes no ``last_valid_month_id`` is a normal, older store. A client that will + not load is not that. It is a broken environment, and returning ``None`` for it would + make the two indistinguishable and ship the unobserved zero-padded tail as observed + history. Same shape as C-60, where a provenance stamp degraded to ``"unknown"`` on a + bare except and made every delivery untraceable. + + This is defence in depth rather than the first line: a missing ``datafactory_query`` + already fails earlier and louder, because the postprocessor's ``config_queryset`` + imports it at module scope and raises, which ``launch_config. + assert_queryset_was_importable`` turns into a refusal before any frame is read + (C-83). Verified 2026-08-17. This guard exists for the paths that gate does not + cover — a direct caller, a future launcher, a partner that does not go through the + same queryset. """ - from datafactory_query.defaults import get_last_valid_month_id + try: + from datafactory_query.defaults import get_last_valid_month_id + except Exception as exc: + # NOT `except ImportError`. The failure this environment has actually had is a + # `ValueError: numpy.dtype size changed ... Expected 96 from C header, got 88` — + # the numpy 1.x/2.x ABI break recorded in views-models + # `postprocessors/un_fao/requirements.txt` on 2026-08-13, found by the + # pre-delivery rehearsal. An ImportError-only clause lets that sail into the + # caller's degrade-open and ship the unobserved tail, which is the exact case + # this guard exists to separate. + if isinstance(exc, ModuleNotFoundError) and (exc.name or "").startswith( + "datafactory_query" + ): + cause = ( + f"it is not installed ({exc.name!r} not found). It ships inside " + "views-datafactory — there is no separate distribution — and the " + "launcher is expected to supply it: " + "pip install 'views-datafactory>=1.9.0,<2.0.0'." + ) + else: + cause = ( + f"it is present but raised while loading: " + f"{type(exc).__name__}: {exc}. Do NOT reinstall views-datafactory on " + "the strength of this — the package is there. Check the environment " + "itself; the known instance is the numpy 1.x/2.x ABI break in the " + "shared envs/views-postprocessing prefix." + ) + err_msg = ( + f"the producer's last_valid_month_id cannot be read because " + f"datafactory_query could not be loaded: {cause} Refusing rather than " + "reporting 'no boundary published' — that is a different condition, and " + "conflating them lets unobserved months ship as observed history " + "(C-103, C-26)." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise ProducerClientUnavailable(err_msg) from exc return get_last_valid_month_id(zarr_url) diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index efb6152..c60996f 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -136,8 +136,18 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except source_metadata.ProducerClientUnavailable: + # NOT degrade-open. A producer client that will not load is a broken + # environment, not a producer that publishes no boundary — and the whole + # point of the two branches is that they are different conditions (C-103). + raise except Exception: - logger.warning("last_valid_month_id unavailable; skipping clip (degrade-open, C-26).", exc_info=True) + logger.warning( + "last_valid_month_id could not be read; skipping the observed-range " + "clip (degrade-open, C-26). Any unobserved months above the producer's " + "boundary WILL ship as observed history in this delivery.", + exc_info=True, + ) lv = None if lv is None: self._historical_frame = frame diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 3b727f8..4f98508 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -136,8 +136,18 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except source_metadata.ProducerClientUnavailable: + # NOT degrade-open. A producer client that will not load is a broken + # environment, not a producer that publishes no boundary — and the whole + # point of the two branches is that they are different conditions (C-103). + raise except Exception: - logger.warning("last_valid_month_id unavailable; skipping clip (degrade-open, C-26).", exc_info=True) + logger.warning( + "last_valid_month_id could not be read; skipping the observed-range " + "clip (degrade-open, C-26). Any unobserved months above the producer's " + "boundary WILL ship as observed history in this delivery.", + exc_info=True, + ) lv = None if lv is None: self._historical_frame = frame