From 38bc17cab32e4d6714d5ec72ea19a4c0e02e2349 Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 15 Sep 2026 10:16:36 +0100 Subject: [PATCH 1/5] feat(search): validate the index before the alias points at it The old sync created a collection, imported batches without reading a single import response, and swapped the alias regardless. A partial import published a half-empty index, and two overlapping DAG runs could prune each other's collections. Publication now checks every import response and the final document count before upserting the alias, and holds a session-scoped advisory lock across the read and the publish so concurrent runs serialise. Cleanup keeps the previous collection as a rollback pointer and is best-effort: an uncertain alias response must never delete what might still be live. Also parses the Typesense URL properly instead of splitting on colons, which mangled any host carrying a scheme and a default port. Co-Authored-By: Claude Opus 5 --- pipeline/scripts/sync_typesense.py | 119 +++++++++++++++----------- pipeline/tests/test_sync_typesense.py | 92 ++++++++++++++++++++ 2 files changed, 161 insertions(+), 50 deletions(-) create mode 100644 pipeline/tests/test_sync_typesense.py diff --git a/pipeline/scripts/sync_typesense.py b/pipeline/scripts/sync_typesense.py index ebe08a3..753c377 100644 --- a/pipeline/scripts/sync_typesense.py +++ b/pipeline/scripts/sync_typesense.py @@ -11,6 +11,10 @@ Usage: from __future__ import annotations import argparse +import json +import logging +import re +import uuid import os import sys import time @@ -112,63 +116,78 @@ def build_document(row: dict) -> dict: return doc +def publish_collection(client, rows: list[dict]) -> str: + """Validate a new collection before moving the alias; keep rollback data. + + Caller holds the database advisory lock across reading and publication so + overlapping school-data DAGs cannot publish or prune each other's work. + Failed drafts are left for the next successful publication to prune: a lost + alias-update response must never cause deletion of a potentially live index. + """ + if not rows or len({r["urn"] for r in rows}) != len(rows): + raise ValueError("Search source must contain nonempty, unique school URNs") + name = f"schools_{int(time.time())}_{uuid.uuid4().hex[:12]}" + try: + previous = client.aliases["schools"].retrieve()["collection_name"] + except typesense.exceptions.ObjectNotFound: + previous = None + client.collections.create({**COLLECTION_SCHEMA, "name": name}) + for i in range(0, len(rows), 500): + batch = [build_document(r) for r in rows[i:i + 500]] + results = client.collections[name].documents.import_(batch, {"action": "upsert"}) + if isinstance(results, str): + results = [json.loads(line) for line in results.splitlines() if line.strip()] + if len(results) != len(batch) or any(r.get("success") is not True for r in results): + raise ValueError("Search import failed; live alias unchanged") + if client.collections[name].retrieve()["num_documents"] != len(rows): + raise ValueError("Search document count mismatch; live alias unchanged") + client.aliases.upsert("schools", {"collection_name": name}) + + # Retention is best-effort and must not make successful publication fail. + try: + keep = {name, previous} + keep.update(a["collection_name"] for a in client.aliases.retrieve()["aliases"]) + for collection in client.collections.retrieve(): + old = collection["name"] + if old not in keep and re.fullmatch(r"schools_\d+(?:_[0-9a-f]+)?", old): + client.collections[old].delete() + except Exception: + logging.getLogger(__name__).exception("Search published, but old collection cleanup failed") + return name + + def sync(typesense_url: str, api_key: str): + from urllib.parse import urlparse + url = urlparse(typesense_url) client = typesense.Client({ - "nodes": [{"host": typesense_url.split("//")[-1].split(":")[0], - "port": typesense_url.split(":")[-1], - "protocol": "http"}], + "nodes": [{"host": url.hostname, "port": str(url.port or 8108), + "protocol": url.scheme}], "api_key": api_key, "connection_timeout_seconds": 10, }) - - # Create timestamped collection for zero-downtime swap - ts = int(time.time()) - collection_name = f"schools_{ts}" - - print(f"Creating collection: {collection_name}") - schema = {**COLLECTION_SCHEMA, "name": collection_name} - client.collections.create(schema) - - # Fetch data from marts — join fact_performance if it exists conn = get_db_connection() - with conn.cursor(cursor_factory=psycopg2.extras.RealDictCursor) as cur: - # Check whether the merged fact table exists - cur.execute(""" - SELECT table_name FROM information_schema.tables - WHERE table_schema = 'marts' AND table_name = 'fact_performance' - """) - has_fact_performance = cur.fetchone() is not None - - query = QUERY_BASE - if has_fact_performance: - query = query.replace( - "l.longitude as lng", - "l.longitude as lng,\n p.rwm_expected_pct,\n p.progress_8_score", - ) - query += QUERY_PERFORMANCE_JOIN - - cur.execute(query) - rows = cur.fetchall() - conn.close() - - print(f"Indexing {len(rows)} schools...") - - # Batch import - batch_size = 500 - for i in range(0, len(rows), batch_size): - batch = [build_document(r) for r in rows[i : i + batch_size]] - client.collections[collection_name].documents.import_(batch, {"action": "upsert"}) - print(f" Indexed {min(i + batch_size, len(rows))}/{len(rows)}") - - # Swap alias - print("Swapping alias 'schools' → new collection") try: - client.aliases.upsert("schools", {"collection_name": collection_name}) - except Exception: - # If alias doesn't exist yet, create it - client.aliases.upsert("schools", {"collection_name": collection_name}) - - print("Done.") + with conn.cursor(cursor_factory=psycopg2.extras.RealDictCursor) as cur: + # Session-scoped lock is released even on errors when conn closes. + cur.execute("SELECT pg_advisory_lock(731042019)") + cur.execute(""" + SELECT table_name FROM information_schema.tables + WHERE table_schema = 'marts' AND table_name = 'fact_performance' + """) + has_fact_performance = cur.fetchone() is not None + query = QUERY_BASE + if has_fact_performance: + query = query.replace( + "l.longitude as lng", + "l.longitude as lng, p.rwm_expected_pct, p.progress_8_score", + ) + query += QUERY_PERFORMANCE_JOIN + cur.execute(query) + rows = cur.fetchall() + name = publish_collection(client, rows) + print(f"Published {len(rows)} schools in {name}") + finally: + conn.close() def main(): diff --git a/pipeline/tests/test_sync_typesense.py b/pipeline/tests/test_sync_typesense.py new file mode 100644 index 0000000..57da930 --- /dev/null +++ b/pipeline/tests/test_sync_typesense.py @@ -0,0 +1,92 @@ +import importlib.util +from pathlib import Path +from unittest.mock import MagicMock +import pytest + + +@pytest.fixture +def sync_module(monkeypatch): + folder = Path(__file__).resolve().parents[1] / 'scripts' + monkeypatch.syspath_prepend(str(folder)) + spec = importlib.util.spec_from_file_location('sync_typesense', folder / 'sync_typesense.py') + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture +def client(): + c = MagicMock() + c.aliases.__getitem__.return_value.retrieve.return_value = {'collection_name': 'schools_2'} + c.aliases.retrieve.return_value = {'aliases': [{'collection_name': 'schools_99'}]} + c.collections.__getitem__.return_value.documents.import_.return_value = [{'success': True}] + c.collections.__getitem__.return_value.retrieve.return_value = {'num_documents': 1} + c.collections.retrieve.return_value = [{'name': n} for n in ['schools_1', 'schools_2', 'schools_99', 'unrelated']] + return c + + +@pytest.fixture +def rows(): + return [{'urn': 100001, 'school_name': 'Example', 'phase_code': 2, + 'school_type_code': 1, 'local_authority': 'Testshire', 'postcode': 'TS1 1AA', + 'total_pupils': 250}] + + +@pytest.mark.parametrize('results', [[{'success': False}], [], [{'success': True}, {'success': True}]]) +def test_partial_import_never_moves_alias(sync_module, client, rows, results): + client.collections.__getitem__.return_value.documents.import_.return_value = results + with pytest.raises(ValueError): + sync_module.publish_collection(client, rows) + client.aliases.upsert.assert_not_called() + client.collections.__getitem__.return_value.delete.assert_not_called() + + +def test_count_mismatch_does_not_publish(sync_module, client, rows): + client.collections.__getitem__.return_value.retrieve.return_value = {'num_documents': 0} + with pytest.raises(ValueError): + sync_module.publish_collection(client, rows) + client.aliases.upsert.assert_not_called() + + +@pytest.mark.parametrize('empty', [True, False]) +def test_invalid_source_never_creates_collection(sync_module, client, rows, empty): + with pytest.raises(ValueError): + sync_module.publish_collection(client, [] if empty else rows + rows) + client.collections.create.assert_not_called() + + +def test_success_retains_previous_and_other_live_aliases(sync_module, client, rows): + # Distinct mock per collection allows checking exactly which one was deleted. + collections = {} + def get(name): + if name not in collections: + c = MagicMock() + c.documents.import_.return_value = '{"success":true}\n' + c.retrieve.return_value = {'num_documents': 1} + collections[name] = c + return collections[name] + client.collections.__getitem__.side_effect = get + name = sync_module.publish_collection(client, rows) + client.aliases.upsert.assert_called_once_with('schools', {'collection_name': name}) + assert set(collections) == {name, 'schools_1'} + collections['schools_1'].delete.assert_called_once() + + +def test_uncertain_alias_update_does_not_delete_candidate(sync_module, client, rows): + client.aliases.upsert.side_effect = RuntimeError('response lost') + with pytest.raises(RuntimeError): + sync_module.publish_collection(client, rows) + client.collections.__getitem__.return_value.delete.assert_not_called() + + +def test_sync_closes_database_when_publication_fails(sync_module, monkeypatch): + conn = MagicMock() + monkeypatch.setattr(sync_module, 'get_db_connection', lambda: conn) + monkeypatch.setattr(sync_module.typesense, 'Client', lambda _: MagicMock()) + def fail(*args): raise ValueError('import rejected') + monkeypatch.setattr(sync_module, 'publish_collection', fail) + with pytest.raises(ValueError): + sync_module.sync('http://localhost:8108', 'dummy') + conn.close.assert_called_once() + statements = [call.args[0] for call in conn.cursor.return_value.__enter__.return_value.execute.call_args_list] + assert statements[0] == 'SELECT pg_advisory_lock(731042019)' -- 2.54.0 From 9b75f542064d3b50f35099240cac65cf47ff21f6 Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 15 Sep 2026 10:16:58 +0100 Subject: [PATCH 2/5] fix(api): publish a validated dataset, and stop truncating search Reload cleared the caches first and rebuilt afterwards, so any failure left the API serving nothing, and requests arriving mid-reload saw a half-swapped state. It now builds and validates the replacement frames, place registry, reverse index and sitemaps off the request loop, then publishes them in one synchronous step under a lock. A failed reload returns 503 and keeps the previous data. Sitemap regeneration takes the same path rather than clearing the live registry up front. Typesense search returned at most one page of hits and used an empty list for both "no matches" and "search is down", so a genuine empty result silently fell back to substring matching. It now pages through every candidate and returns None only on failure; the fallback matches literally, since a query containing regex metacharacters used to throw. Empty datasets answer 503 rather than 200-with-nothing or a misleading 404, so callers can tell an outage from an absent school. Adds /api/release, which reports the build identity baked into the image. Co-Authored-By: Claude Opus 5 --- backend/app.py | 111 +++++++++++++++------- backend/data_loader.py | 48 +++++++--- backend/tests/test_publication.py | 68 +++++++++++++ backend/tests/test_search_completeness.py | 73 ++++++++++++++ 4 files changed, 253 insertions(+), 47 deletions(-) create mode 100644 backend/tests/test_publication.py create mode 100644 backend/tests/test_search_completeness.py diff --git a/backend/app.py b/backend/app.py index caa3c21..e5d5b62 100644 --- a/backend/app.py +++ b/backend/app.py @@ -26,7 +26,8 @@ from starlette.middleware.base import BaseHTTPMiddleware import asyncio from .config import settings from .data_loader import ( - clear_cache, + build_latest_school_data, + load_school_data_as_dataframe, compute_benchmarks, load_school_data, load_latest_school_data, @@ -272,7 +273,7 @@ def _places_payload(urn: int) -> list[dict]: return payload -def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]: +def _place_sitemap_rows(kinds: tuple[str, ...], registry=None) -> list[str]: """A per place, plus a phase variant wherever that phase clears the threshold on its own. @@ -282,7 +283,9 @@ def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]: linked from the place page either. """ rows: list[str] = [] - for p in sorted(get_place_registry().values(), key=lambda p: (p.kind, p.slug)): + if registry is None: + registry = get_place_registry() + for p in sorted(registry.values(), key=lambda p: (p.kind, p.slug)): if p.kind not in kinds: continue rows.append(_url_element(BASE_URL + _place_url(p))) @@ -296,9 +299,10 @@ def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]: return rows -def build_sitemaps() -> dict[str, str]: +def build_sitemaps(df=None, registry=None) -> dict[str, str]: """Build the sitemap index and every child, keyed by name.""" - df = load_school_data() + if df is None: + df = load_school_data() children: dict[str, str] = { "static.xml": _urlset( @@ -318,7 +322,7 @@ def build_sitemaps() -> dict[str, str]: # measured apart from the school pages'. for label, kinds in (("places", ("town", "locality", "authority")), ("outcodes", ("outcode",))): - rows = _place_sitemap_rows(kinds) + rows = _place_sitemap_rows(kinds, registry) chunks = [rows[i:i + SITEMAP_CHUNK_SIZE] for i in range(0, len(rows), SITEMAP_CHUNK_SIZE)] or [[]] for n, chunk in enumerate(chunks, start=1): @@ -700,6 +704,15 @@ async def get_config(): } +@app.get("/api/release") +async def release_identity(): + import json + from pathlib import Path + path = Path(__file__).with_name("build-info.json") + identity = json.loads(path.read_text()) if path.exists() else {"sha": "development", "build_id": "development"} + return JSONResponse(identity, headers={"Cache-Control": "no-store"}) + + @app.get("/api/schools") @limiter.limit(f"{settings.rate_limit_per_minute}/minute") async def get_schools( @@ -736,7 +749,7 @@ async def get_schools( df_latest = load_latest_school_data() if df_latest.empty: - return {"schools": [], "total": 0, "page": page, "page_size": 0} + raise HTTPException(status_code=503, detail="School data temporarily unavailable") # Use configured default if not specified if page_size is None: @@ -835,8 +848,8 @@ async def get_schools( # Apply filters if search: - ts_urns = search_schools_typesense(search) - if ts_urns: + ts_urns = await asyncio.to_thread(search_schools_typesense, search) + if ts_urns is not None: urn_order = {urn: i for i, urn in enumerate(ts_urns)} schools_df = schools_df[schools_df["urn"].isin(set(ts_urns))].copy() schools_df["_ts_rank"] = schools_df["urn"].map(urn_order) @@ -844,9 +857,9 @@ async def get_schools( else: # Fallback: Typesense unavailable, use substring match search_lower = search.lower() - mask = schools_df["school_name"].str.lower().str.contains(search_lower, na=False) + mask = schools_df["school_name"].str.lower().str.contains(search_lower, na=False, regex=False) if "address" in schools_df.columns: - mask = mask | schools_df["address"].str.lower().str.contains(search_lower, na=False) + mask = mask | schools_df["address"].str.lower().str.contains(search_lower, na=False, regex=False) schools_df = schools_df[mask] if local_authority: @@ -905,7 +918,7 @@ async def get_school_details(request: Request, urn: int): df = load_school_data() if df.empty: - raise HTTPException(status_code=404, detail="No data available") + raise HTTPException(status_code=503, detail="School data temporarily unavailable") school_data = df[df["urn"] == urn] @@ -1542,20 +1555,51 @@ async def get_data_info(request: Request): } +_publication_lock = asyncio.Lock() + + +def _prepare_publication(df): + if df.empty: + raise ValueError("Refusing to publish an empty school dataset") + if not {"urn", "year", "school_name"}.issubset(df.columns): + raise ValueError("School dataset is missing required columns") + if df["urn"].isna().any() or df.duplicated(["urn", "year"]).any(): + raise ValueError("School dataset has missing URNs or duplicate school years") + latest = build_latest_school_data(df) + registry = build_place_registry(df) + index = build_place_index(registry) + sitemaps = build_sitemaps(df, registry) + return df, latest, registry, index, sitemaps + + +def _publish(prepared): + # Called on the event loop with no await: routes cannot observe half a swap. + # The application currently runs one worker; replicas require coordination. + from . import data_loader + global _place_registry, _place_index, _place_index_source, _sitemaps + df, latest, registry, index, sitemaps = prepared + data_loader._df_cache = df + data_loader._df_latest_cache = latest + _place_registry = registry + _place_index = index + _place_index_source = registry + _sitemaps = sitemaps + + @app.post("/api/admin/reload") @limiter.limit("5/minute") -async def reload_data( - request: Request, - _: bool = Depends(verify_admin_api_key) -): - """ - Admin endpoint to force data reload (useful after data updates). - Requires X-API-Key header with valid admin API key. - """ - clear_cache() - await asyncio.to_thread(load_school_data) - await asyncio.to_thread(load_latest_school_data) - return {"status": "reloaded"} +async def reload_data(request: Request, _: bool = Depends(verify_admin_api_key)): + """Validate a complete replacement before publishing it; retain data on failure.""" + async with _publication_lock: + try: + df = await asyncio.to_thread(load_school_data_as_dataframe) + prepared = await asyncio.to_thread(_prepare_publication, df) + except Exception as exc: + import logging + logging.getLogger(__name__).exception("Dataset reload failed") + raise HTTPException(status_code=503, detail="Dataset reload failed; previous data retained") from exc + _publish(prepared) + return {"status": "reloaded", "schools": len(prepared[1])} @@ -1607,15 +1651,16 @@ async def regenerate_sitemap( request: Request, _: bool = Depends(verify_admin_api_key), ): - """Rebuild and cache the sitemap from current school data. Called by Airflow after data updates.""" - global _sitemaps, _place_registry - # Places and sitemap are rebuilt together — they read the same marts, and - # letting them drift apart would submit URLs for places that no longer - # exist. - _place_registry = None - _sitemaps = build_sitemaps() - n = sum(x.count("") for x in _sitemaps.values()) - return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)} + """Rebuild derived publication data without clearing the live registry.""" + async with _publication_lock: + try: + prepared = await asyncio.to_thread(_prepare_publication, load_school_data()) + except Exception as exc: + raise HTTPException(status_code=503, detail="Sitemap rebuild failed; previous data retained") from exc + _publish(prepared) + n = sum(x.count("") for x in prepared[4].values()) + return {"status": "ok", "urls": n, "sitemaps": len(prepared[4])} + # Mount static files directly (must be after all routes to avoid catching API calls) diff --git a/backend/data_loader.py b/backend/data_loader.py index ebd8d05..1816745 100644 --- a/backend/data_loader.py +++ b/backend/data_loader.py @@ -84,21 +84,37 @@ def _get_typesense_client(): return None -def search_schools_typesense(query: str, limit: int = 250) -> List[int]: - """Search Typesense. Returns URNs in relevance order, or [] if unavailable.""" +def search_schools_typesense(query: str) -> Optional[List[int]]: + """Return all matching URNs in relevance order; None means unavailable. + + Filtering and user pagination happen in the API after this search. Returning + only the first search page would silently discard valid local matches. + Never return a partial candidate set if a later page fails. + """ client = _get_typesense_client() if client is None: - return [] + return None + urns = [] try: - result = client.collections["schools"].documents.search({ - "q": query, - "query_by": "school_name,local_authority,postcode", - "per_page": min(limit, 250), - "typo_tokens_threshold": 1, - }) - return [int(h["document"]["urn"]) for h in result.get("hits", [])] + page = 1 + while True: + result = client.collections["schools"].documents.search({ + "q": query, + "query_by": "school_name,local_authority,postcode", + "per_page": 250, + "page": page, + "typo_tokens_threshold": 1, + }) + hits = result.get("hits", []) + urns.extend(int(h["document"]["urn"]) for h in hits) + if len(urns) >= result.get("found", len(urns)): + return list(dict.fromkeys(urns)) + if not hits: + raise ValueError("Search pagination ended before all matches arrived") + page += 1 except Exception: - return [] + logging.getLogger(__name__).exception("School search unavailable") + return None # The most a public endpoint will return in one response. @@ -502,7 +518,12 @@ def load_latest_school_data() -> pd.DataFrame: if _df_latest_cache is not None: return _df_latest_cache - df = load_school_data() + _df_latest_cache = build_latest_school_data(load_school_data()) + return _df_latest_cache + + +def build_latest_school_data(df: pd.DataFrame) -> pd.DataFrame: + """Build a replacement snapshot without mutating the published caches.""" if df.empty: return df @@ -535,8 +556,7 @@ def load_latest_school_data() -> pd.DataFrame: df_latest = pd.concat([df_latest, df_no_perf], ignore_index=True) print(f"Latest-snapshot cache built: {len(df_latest)} schools") - _df_latest_cache = df_latest - return _df_latest_cache + return df_latest def clear_cache(): diff --git a/backend/tests/test_publication.py b/backend/tests/test_publication.py new file mode 100644 index 0000000..07a20bb --- /dev/null +++ b/backend/tests/test_publication.py @@ -0,0 +1,68 @@ +"""Publication must preserve the current dataset until every replacement is ready.""" +import asyncio +import pandas as pd +import pytest +from fastapi.testclient import TestClient +from backend import app as api, data_loader +from backend.tests.test_sixth_form_flag import _schools_df + + +@pytest.fixture +def client(monkeypatch): + old = _schools_df() + monkeypatch.setattr(data_loader, '_df_cache', old) + monkeypatch.setattr(data_loader, '_df_latest_cache', old) + monkeypatch.setattr(api, '_place_registry', {'old': 'registry'}) + monkeypatch.setattr(api, '_place_index', {'old': 'index'}) + monkeypatch.setattr(api, '_place_index_source', api._place_registry) + monkeypatch.setattr(api, '_sitemaps', {'old.xml': 'old sitemap'}) + monkeypatch.setattr(api, '_publication_lock', asyncio.Lock()) + monkeypatch.setattr(api.limiter, 'enabled', False) + api.app.dependency_overrides[api.verify_admin_api_key] = lambda: True + yield TestClient(api.app, raise_server_exceptions=False) + api.app.dependency_overrides.clear() + + +def state(): + return (data_loader._df_cache, data_loader._df_latest_cache, api._place_registry, + api._place_index, api._place_index_source, api._sitemaps) + + +@pytest.mark.parametrize('failure', ['empty', 'database', 'sitemap', 'duplicate']) +def test_failed_reload_preserves_every_published_object(client, monkeypatch, failure): + before = state() + df = _schools_df() + if failure == 'empty': + df = pd.DataFrame() + if failure == 'duplicate': + df = pd.concat([df, df.iloc[:1]], ignore_index=True) + def load(): + if failure == 'database': + raise RuntimeError('database unavailable') + return df + monkeypatch.setattr(api, 'load_school_data_as_dataframe', load) + if failure == 'sitemap': + monkeypatch.setattr(api, 'build_sitemaps', lambda *args: (_ for _ in ()).throw(RuntimeError('bad XML'))) + response = client.post('/api/admin/reload') + assert response.status_code == 503 + assert all(a is b for a, b in zip(before, state())) + + +def test_success_publishes_school_data_places_and_sitemaps(client, monkeypatch): + df = _schools_df() + df.loc[0, 'school_name'] = 'Replacement School' + monkeypatch.setattr(api, 'load_school_data_as_dataframe', lambda: df) + response = client.post('/api/admin/reload') + assert response.status_code == 200 + assert data_loader.load_school_data() is df + assert data_loader.load_latest_school_data().iloc[0].school_name == 'Replacement School' + assert api._place_index_source is api._place_registry + assert 'old.xml' not in api._sitemaps + assert 'replacement-school' in api._sitemaps['schools-1.xml'] + + +def test_failed_sitemap_regeneration_keeps_existing_publication(client, monkeypatch): + before = state() + monkeypatch.setattr(api, 'build_sitemaps', lambda *args: (_ for _ in ()).throw(RuntimeError('bad XML'))) + assert client.post('/api/admin/regenerate-sitemap').status_code == 503 + assert all(a is b for a, b in zip(before, state())) diff --git a/backend/tests/test_search_completeness.py b/backend/tests/test_search_completeness.py new file mode 100644 index 0000000..e170a8b --- /dev/null +++ b/backend/tests/test_search_completeness.py @@ -0,0 +1,73 @@ +from types import SimpleNamespace +import pytest +from fastapi.testclient import TestClient +from backend import app as api, data_loader +from backend.tests.test_sixth_form_flag import _schools_df + + +def client_for(monkeypatch, search): + client = SimpleNamespace(collections={'schools': SimpleNamespace(documents=SimpleNamespace(search=search))}) + monkeypatch.setattr(data_loader, '_get_typesense_client', lambda: client) + + +def test_search_returns_matches_beyond_first_page(monkeypatch): + pages = [] + def search(params): + pages.append(params['page']) + urns = range(100000, 100250) if params['page'] == 1 else [100999] + return {'found': 251, 'hits': [{'document': {'urn': u}} for u in urns]} + client_for(monkeypatch, search) + result = data_loader.search_schools_typesense('academy') + assert len(result) == 251 + assert result[-1] == 100999 + assert pages == [1, 2] + + +def test_later_page_failure_does_not_return_partial_results(monkeypatch): + def search(params): + if params['page'] == 2: + raise RuntimeError('timeout') + return {'found': 251, 'hits': [{'document': {'urn': u}} for u in range(100000, 100250)]} + client_for(monkeypatch, search) + assert data_loader.search_schools_typesense('academy') is None + + +def test_zero_matches_are_distinct_from_unavailable(monkeypatch): + client_for(monkeypatch, lambda _: {'found': 0, 'hits': []}) + assert data_loader.search_schools_typesense('academy') == [] + monkeypatch.setattr(data_loader, '_get_typesense_client', lambda: None) + assert data_loader.search_schools_typesense('academy') is None + + +@pytest.mark.parametrize('matches, expected', [([], []), (None, [100001])]) +def test_fallback_only_on_dependency_failure(monkeypatch, matches, expected): + monkeypatch.setattr(api.limiter, 'enabled', False) + monkeypatch.setattr(api, 'load_latest_school_data', _schools_df) + monkeypatch.setattr(api, 'search_schools_typesense', lambda _: matches) + response = TestClient(api.app).get('/api/schools?search=Alpha') + assert response.status_code == 200 + assert [s['urn'] for s in response.json()['schools']] == expected + + +def test_filtered_api_keeps_match_from_second_search_page(monkeypatch): + monkeypatch.setattr(api.limiter, 'enabled', False) + df = _schools_df() + monkeypatch.setattr(api, 'load_latest_school_data', lambda: df) + def search(params): + urns = range(200000, 200250) if params['page'] == 1 else [100001] + return {'found': 251, 'hits': [{'document': {'urn': u}} for u in urns]} + client_for(monkeypatch, search) + response = TestClient(api.app).get('/api/schools?search=Alpha&local_authority=Testshire') + assert response.status_code == 200 + assert response.json()['total'] == 1 + assert response.json()['schools'][0]['urn'] == 100001 + + +def test_unavailable_dataset_is_not_a_missing_school_or_empty_search(monkeypatch): + import pandas as pd + monkeypatch.setattr(api.limiter, 'enabled', False) + monkeypatch.setattr(api, 'load_school_data', lambda: pd.DataFrame()) + monkeypatch.setattr(api, 'load_latest_school_data', lambda: pd.DataFrame()) + client = TestClient(api.app) + assert client.get('/api/schools/100001').status_code == 503 + assert client.get('/api/schools?search=school').status_code == 503 -- 2.54.0 From 7b41218e6e141154b1b08fdf5762f59f9a041830 Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 15 Sep 2026 10:17:18 +0100 Subject: [PATCH 3/5] fix(web): show an outage as an outage, and drop superseded fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The home page caught every fetch failure and rendered its empty state, so a backend outage looked like a site with no schools in it. School pages turned any error into notFound(), which told visitors — and crawlers — that a real school had ceased to exist. Place fetches did the same by returning [] and null. Failures now reach a retryable error boundary; only a genuine 404 still calls notFound(). "Load more" and the map fetch resolved against whatever state existed when they returned, so results from an abandoned search appended themselves to the new ones. Each fetch now carries an AbortController and checks that its search scope is still current before touching state. The map only records its cache key on success, so a failed load retries instead of pinning the stale marker set. Jest ignored .next/, whose build output otherwise shadowed real suites. Co-Authored-By: Claude Opus 5 --- .../__tests__/app/dataFailures.test.tsx | 51 ++++++++ .../components/HomeView.staleFetch.test.tsx | 81 +++++++++++++ nextjs-app/app/(frontend)/error.tsx | 12 ++ nextjs-app/app/(frontend)/page.tsx | 111 +++++++----------- .../app/(frontend)/school/[slug]/page.tsx | 6 +- nextjs-app/components/HomeView.tsx | 42 +++++-- nextjs-app/jest.config.cjs | 1 + nextjs-app/lib/places.ts | 7 +- 8 files changed, 231 insertions(+), 80 deletions(-) create mode 100644 nextjs-app/__tests__/app/dataFailures.test.tsx create mode 100644 nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx create mode 100644 nextjs-app/app/(frontend)/error.tsx diff --git a/nextjs-app/__tests__/app/dataFailures.test.tsx b/nextjs-app/__tests__/app/dataFailures.test.tsx new file mode 100644 index 0000000..95ba9d4 --- /dev/null +++ b/nextjs-app/__tests__/app/dataFailures.test.tsx @@ -0,0 +1,51 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import HomePage from '@/app/(frontend)/page'; +import SchoolPage from '@/app/(frontend)/school/[slug]/page'; +import ErrorPage from '@/app/(frontend)/error'; +import { APIFetchError, fetchSchools, fetchFilters, fetchSchoolDetails } from '@/lib/api'; +import { fetchPlace, fetchPlaces } from '@/lib/places'; + +jest.mock('@/lib/api', () => ({ + ...jest.requireActual('@/lib/api'), + fetchSchools: jest.fn(), + fetchSchoolDetails: jest.fn(), + fetchFilters: jest.fn(async () => ({})), + fetchDataInfo: jest.fn(async () => null), + fetchNationalAverages: jest.fn(async () => null), +})); +jest.mock('@/lib/flags', () => ({ getFlags: jest.fn(async () => ({})) })); +jest.mock('next/navigation', () => ({ + notFound: () => { throw new Error('NEXT_NOT_FOUND'); }, + redirect: jest.fn(), +})); +const realFetch = global.fetch; +afterEach(() => { global.fetch = realFetch; jest.clearAllMocks(); }); + +test('school outages propagate; only a real 404 becomes not found', async () => { + const request = { params: Promise.resolve({ slug: '100001-school' }) }; + const outage = new APIFetchError('unavailable', 503); + jest.mocked(fetchSchoolDetails).mockRejectedValueOnce(outage); + await expect(SchoolPage(request)).rejects.toBe(outage); + jest.mocked(fetchSchoolDetails).mockRejectedValueOnce(new APIFetchError('missing', 404)); + await expect(SchoolPage(request)).rejects.toThrow('NEXT_NOT_FOUND'); +}); + +test('homepage search failure is not returned as an empty successful page', async () => { + jest.mocked(fetchSchools).mockRejectedValueOnce(new APIFetchError('unavailable', 503)); + await expect(HomePage({ searchParams: Promise.resolve({ search: 'school' }) })).rejects.toThrow('unavailable'); +}); + +test('a place is absent only on 404; other failures propagate', async () => { + global.fetch = jest.fn().mockResolvedValue({ ok: false, status: 404 }); + await expect(fetchPlace('town', 'example')).resolves.toBeNull(); + jest.mocked(global.fetch).mockResolvedValue({ ok: false, status: 503 } as Response); + await expect(fetchPlace('town', 'example')).rejects.toMatchObject({ status: 503 }); + await expect(fetchPlaces()).rejects.toMatchObject({ status: 503 }); +}); + +test('the error boundary offers a retry without showing an empty search', () => { + const reset = jest.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: 'Try again' })); + expect(reset).toHaveBeenCalledTimes(1); +}); diff --git a/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx b/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx new file mode 100644 index 0000000..4b35b3f --- /dev/null +++ b/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx @@ -0,0 +1,81 @@ +import { act, fireEvent, render, screen } from '@testing-library/react'; +import { HomeView } from '@/components/HomeView'; +import { fetchSchools } from '@/lib/api'; +import { primaryFixture } from '../support/schoolFixtures'; +import type { SchoolsResponse, School } from '@/lib/types'; + +let params = new URLSearchParams('postcode=SW1A+1AA'); +jest.mock('next/navigation', () => ({ + useSearchParams: () => params, + usePathname: () => '/', + useRouter: () => ({ push: jest.fn(), replace: jest.fn() }), +})); +jest.mock('@/context/ComparisonContext', () => ({ + useComparisonContext: () => ({ addSchool: jest.fn(), removeSchool: jest.fn(), selectedSchools: [] }), +})); +jest.mock('@/lib/api', () => ({ + fetchSchools: jest.fn(), + fetchNationalAverages: jest.fn(async () => ({})), + fetchLAaverages: jest.fn(async () => ({ secondary: { attainment_8_by_la: {} } })), +})); +jest.mock('@/components/FilterBar', () => ({ FilterBar: () => null })); +jest.mock('@/components/SchoolRow', () => ({ SchoolRow: ({ school }: {school: School}) =>
{school.school_name}
})); +jest.mock('@/components/SchoolMap', () => ({ SchoolMap: ({ schools }: {schools: School[]}) =>
{schools.map(s => s.school_name).join(',')}
})); + +const filters = { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; +function response(name: string): SchoolsResponse { + return { schools: [{ ...primaryFixture.schoolInfo, school_name: name }], + total: 2, page: 1, page_size: 1, total_pages: 2 }; +} +function deferred() { + let resolve!: (value: SchoolsResponse) => void; + let reject!: (error: Error) => void; + const promise = new Promise((yes, no) => { resolve = yes; reject = no; }); + return { promise, resolve, reject }; +} +beforeEach(() => { + params = new URLSearchParams('postcode=SW1A+1AA'); + jest.mocked(fetchSchools).mockReset(); +}); + +test('load-more results from an old search are discarded, even after returning to it', async () => { + const pending = deferred(); + jest.mocked(fetchSchools).mockReturnValueOnce(pending.promise); + const view = render(); + fireEvent.click(screen.getByRole('button', { name: 'Load more schools' })); + const signal = jest.mocked(fetchSchools).mock.calls[0][1]?.signal; + params = new URLSearchParams('postcode=SW2+1AA'); + view.rerender(); + expect(signal?.aborted).toBe(true); + params = new URLSearchParams('postcode=SW1A+1AA'); + view.rerender(); + await act(async () => pending.resolve(response('Stale append'))); + expect(screen.queryByText('Stale append')).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Load more schools' })).toBeEnabled(); +}); + +test('an older map response cannot overwrite the current search', async () => { + const first = deferred(), second = deferred(); + jest.mocked(fetchSchools).mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise); + const initial = response('Initial A'); + const view = render(); + fireEvent.click(screen.getByRole('button', { name: 'Map' })); + params = new URLSearchParams('postcode=SW2+1AA'); + view.rerender(); + await act(async () => second.resolve(response('Current map'))); + await act(async () => first.resolve(response('Stale map'))); + expect(screen.getByTestId('map')).toHaveTextContent('Current map'); + expect(screen.getByTestId('map')).not.toHaveTextContent('Stale map'); +}); + +test('failed map requests can be retried by reopening the map', async () => { + const pending = deferred(); + jest.mocked(fetchSchools).mockReturnValueOnce(pending.promise).mockResolvedValue(response('Retry result')); + render(); + fireEvent.click(screen.getByRole('button', { name: 'Map' })); + await act(async () => pending.reject(new Error('offline'))); + fireEvent.click(screen.getByRole('button', { name: 'List' })); + await act(async () => fireEvent.click(screen.getByRole('button', { name: 'Map' }))); + expect(fetchSchools).toHaveBeenCalledTimes(2); + expect(screen.getByTestId('map')).toHaveTextContent('Retry result'); +}); diff --git a/nextjs-app/app/(frontend)/error.tsx b/nextjs-app/app/(frontend)/error.tsx new file mode 100644 index 0000000..134d2f2 --- /dev/null +++ b/nextjs-app/app/(frontend)/error.tsx @@ -0,0 +1,12 @@ +'use client'; + +export default function ErrorPage({ reset }: { error: Error & { digest?: string }; reset: () => void }) { + return ( +
+

We couldn’t load this page

+

School information is temporarily unavailable. Please try again.

+ +

Return to school search

+
+ ); +} diff --git a/nextjs-app/app/(frontend)/page.tsx b/nextjs-app/app/(frontend)/page.tsx index f791b95..f3fd365 100644 --- a/nextjs-app/app/(frontend)/page.tsx +++ b/nextjs-app/app/(frontend)/page.tsx @@ -85,73 +85,50 @@ export default async function HomePage({ searchParams }: HomePageProps) { params.has_sixth_form ); - // Fetch data on server with error handling - try { - const [filtersData, dataInfo] = await Promise.all([fetchFilters(), fetchDataInfo().catch(() => null)]); + // Failures propagate to the retryable error boundary. + const [filtersData, dataInfo] = await Promise.all([fetchFilters(), fetchDataInfo().catch(() => null)]); - // Only fetch schools if there are search parameters - let schoolsData; - if (hasSearchParams) { - schoolsData = await fetchSchools({ - search: params.search, - local_authority: params.local_authority, - school_type: params.school_type, - phase: params.phase, - postcode: params.postcode, - radius, - page, - page_size: 50, - gender: params.gender, - admissions_policy: params.admissions_policy, - has_sixth_form: params.has_sixth_form, - }); - } else { - // Empty state by default - schoolsData = { schools: [], page: 1, page_size: 50, total: 0, total_pages: 0 }; - } - - const resolvedFilters = filtersData || { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; - // `unique_schools`, not `total_schools` — the latter is not a field this - // endpoint returns, and reading it silently yielded null on every request. - const total = dataInfo?.unique_schools ?? null; - const years = dataInfo?.years_available ?? []; - return ( - } - editorial={hasSearchParams ? null : ( - - )} - /> - ); - } catch (error) { - console.error('Error fetching data for home page:', error); - - const emptyFilters = { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; - return ( - } - editorial={hasSearchParams ? null : ( - - )} - /> - ); + // Only fetch schools if there are search parameters + let schoolsData; + if (hasSearchParams) { + schoolsData = await fetchSchools({ + search: params.search, + local_authority: params.local_authority, + school_type: params.school_type, + phase: params.phase, + postcode: params.postcode, + radius, + page, + page_size: 50, + gender: params.gender, + admissions_policy: params.admissions_policy, + has_sixth_form: params.has_sixth_form, + }); + } else { + // Empty state by default + schoolsData = { schools: [], page: 1, page_size: 50, total: 0, total_pages: 0 }; } + + const resolvedFilters = filtersData || { local_authorities: [], school_types: [], years: [], phases: [], genders: [], admissions_policies: [] }; + // `unique_schools`, not `total_schools` — the latter is not a field this + // endpoint returns, and reading it silently yielded null on every request. + const total = dataInfo?.unique_schools ?? null; + const years = dataInfo?.years_available ?? []; + return ( + } + editorial={hasSearchParams ? null : ( + + )} + /> + ); } diff --git a/nextjs-app/app/(frontend)/school/[slug]/page.tsx b/nextjs-app/app/(frontend)/school/[slug]/page.tsx index 9b3649f..fe0f4af 100644 --- a/nextjs-app/app/(frontend)/school/[slug]/page.tsx +++ b/nextjs-app/app/(frontend)/school/[slug]/page.tsx @@ -4,7 +4,7 @@ * URL format: /school/138267-school-name-here */ -import { fetchSchoolDetails, fetchSchools, fetchNationalAverages } from '@/lib/api'; +import { APIFetchError, fetchSchoolDetails, fetchSchools, fetchNationalAverages } from '@/lib/api'; import { notFound, redirect } from 'next/navigation'; import { SchoolDetailShell } from '@/components/school/SchoolDetailShell'; import { NearbyPlaces } from '@/components/school/NearbyPlaces'; @@ -146,8 +146,8 @@ export default async function SchoolPage({ params }: SchoolPageProps) { fetchNationalAverages().catch(() => null), ]); } catch (error) { - console.error(`Failed to fetch school ${urn}:`, error); - notFound(); + if (error instanceof APIFetchError && error.status === 404) notFound(); + throw error; } const { school_info, yearly_data, absence_data, ofsted, census, admissions, admissions_history, admission_distance, deprivation, finance, destinations } = data; diff --git a/nextjs-app/components/HomeView.tsx b/nextjs-app/components/HomeView.tsx index fd90e77..66632f4 100644 --- a/nextjs-app/components/HomeView.tsx +++ b/nextjs-app/components/HomeView.tsx @@ -213,6 +213,16 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed const [isLoadingMap, setIsLoadingMap] = useState(false); const prevSearchParamsRef = useRef(searchParams.toString()); const mapParamsRef = useRef(''); + const loadMoreController = useRef(null); + // Identity changes even for A → B → A, so an old A response stays stale. + const searchScope = useRef({ key: searchParams.toString() }); + if (searchScope.current.key !== searchParams.toString()) { + searchScope.current = { key: searchParams.toString() }; + } + useEffect(() => { + setIsLoadingMore(false); + return () => { loadMoreController.current?.abort(); }; + }, [searchParams]); const [geoState, setGeoState] = useState<'idle' | 'requesting' | 'error'>('idle'); const [geoError, setGeoError] = useState(null); /* @@ -274,17 +284,27 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed if (resultsView !== 'map' || !isLocationSearch) return; const paramsKey = searchParams.toString(); if (paramsKey === mapParamsRef.current) return; - mapParamsRef.current = paramsKey; + const controller = new AbortController(); + const scope = searchScope.current; + const current = () => !controller.signal.aborted && searchScope.current === scope; setIsLoadingMap(true); const params: Record = {}; searchParams.forEach((value, key) => { params[key] = value; }); params.page = 1; params.page_size = 500; - fetchSchools(params, { cache: 'no-store' }) - .then(r => setMapSchools(r.schools)) - .catch(() => setMapSchools(initialSchools.schools)) - .finally(() => setIsLoadingMap(false)); - }, [resultsView, searchParams]); + fetchSchools(params, { cache: 'no-store', signal: controller.signal }) + .then(r => { + if (!current()) return; + mapParamsRef.current = paramsKey; + setMapSchools(r.schools); + }) + .catch(() => { + if (current()) setMapSchools(initialSchools.schools); + // No cache marker on failure: opening the map again retries. + }) + .finally(() => { if (current()) setIsLoadingMap(false); }); + return () => controller.abort(); + }, [resultsView, searchParams, initialSchools.schools]); // Fetch LA averages when secondary or mixed schools are visible useEffect(() => { @@ -305,19 +325,25 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed if (isLoadingMore || !hasMore) return; track('results_load_more', { next_page: currentPage + 1 }); setIsLoadingMore(true); + const scope = searchScope.current; + const controller = new AbortController(); + loadMoreController.current?.abort(); + loadMoreController.current = controller; + const current = () => !controller.signal.aborted && searchScope.current === scope; try { const params: Record = {}; searchParams.forEach((value, key) => { params[key] = value; }); params.page = currentPage + 1; params.page_size = initialSchools.page_size; - const response = await fetchSchools(params, { cache: 'no-store' }); + const response = await fetchSchools(params, { cache: 'no-store', signal: controller.signal }); + if (!current()) return; setAllSchools(prev => [...prev, ...response.schools]); setCurrentPage(response.page); setHasMore(response.page < response.total_pages); } catch { // silently ignore } finally { - setIsLoadingMore(false); + if (current()) setIsLoadingMore(false); } }; diff --git a/nextjs-app/jest.config.cjs b/nextjs-app/jest.config.cjs index 4bb2bb1..37fee00 100644 --- a/nextjs-app/jest.config.cjs +++ b/nextjs-app/jest.config.cjs @@ -9,6 +9,7 @@ const createJestConfig = nextJest({ const customJestConfig = { setupFilesAfterEnv: ['/jest.setup.js'], testEnvironment: 'jest-environment-jsdom', + modulePathIgnorePatterns: ['/.next/'], moduleNameMapper: { '^@/(.*)$': '/$1', }, diff --git a/nextjs-app/lib/places.ts b/nextjs-app/lib/places.ts index 29529f4..143fb7b 100644 --- a/nextjs-app/lib/places.ts +++ b/nextjs-app/lib/places.ts @@ -1,3 +1,5 @@ +import { APIFetchError } from './api'; + /** * Client for the places API. * @@ -69,7 +71,7 @@ const API = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL export async function fetchPlaces(): Promise { const res = await fetch(`${API}/places`, { next: { revalidate: 604800 } }); - if (!res.ok) return []; + if (!res.ok) throw new APIFetchError("Unable to load places", res.status); return (await res.json()).places ?? []; } @@ -79,6 +81,7 @@ export async function fetchPlace( const q = phase ? `?phase=${encodeURIComponent(phase)}` : ''; const res = await fetch(`${API}/places/${kind}/${slug}${q}`, { next: { revalidate: 604800 } }); - if (!res.ok) return null; + if (res.status === 404) return null; + if (!res.ok) throw new APIFetchError("Unable to load place", res.status); return res.json(); } -- 2.54.0 From 0c901cd0d1fe404334b034a16b49b9eafef2d22b Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 15 Sep 2026 10:17:50 +0100 Subject: [PATCH 4/5] feat(ci): gate promotion on the image set that actually passed E2E MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Staging health polling asked only whether something answered HTTP 200 at the base URL. It could not tell the new deployment from the old one, so journeys could pass against the previous release, and concurrent merges could move the staging tags underneath a run in flight. Each staging run now mints a build ID and stamps all three images with the commit and that ID, as labels and — for frontend and backend — as a build-time JSON file that environment overrides cannot rewrite. /release.json reports both identities uncached, and scripts/ci/release.py polls for the expected pair before and after the journeys. Only then are the captured build digests tagged verified-. Promotion resolves those verified tags to immutable digests, revalidates their labels, and refuses a mixed or incomplete set before any :prod tag moves. The whole staging workflow shares one concurrency group with cancellation disabled, so releases serialise. The scripts are stdlib-only and unit-tested against mocked registry and HTTP behaviour; PR checks now run the pipeline and CI suites too. The runbook records what this cannot prove locally, and that the first rollout needs a commit built by this workflow. Co-Authored-By: Claude Opus 5 --- .gitea/workflows/deploy.yml | 90 +++++++++--- .gitea/workflows/pr-checks.yml | 4 +- .gitea/workflows/promote.yml | 32 +---- Dockerfile | 6 + docs/ARCHITECTURE.md | 33 +++-- docs/DEPLOY.md | 48 ++++++- docs/DEVELOPMENT.md | 4 +- e2e/tests/reliability.spec.ts | 41 ++++++ nextjs-app/Dockerfile | 10 ++ .../__tests__/app/releaseIdentity.test.ts | 30 ++++ .../app/(frontend)/release.json/route.ts | 17 +++ pipeline/Dockerfile | 5 + scripts/ci/release.py | 136 ++++++++++++++++++ scripts/ci/tests/test_release.py | 65 +++++++++ scripts/ci/tests/test_workflows.py | 34 +++++ 15 files changed, 494 insertions(+), 61 deletions(-) create mode 100644 e2e/tests/reliability.spec.ts create mode 100644 nextjs-app/__tests__/app/releaseIdentity.test.ts create mode 100644 nextjs-app/app/(frontend)/release.json/route.ts create mode 100644 scripts/ci/release.py create mode 100644 scripts/ci/tests/test_release.py create mode 100644 scripts/ci/tests/test_workflows.py diff --git a/.gitea/workflows/deploy.yml b/.gitea/workflows/deploy.yml index 31bb6ea..f24cb71 100644 --- a/.gitea/workflows/deploy.yml +++ b/.gitea/workflows/deploy.yml @@ -5,6 +5,11 @@ on: branches: - main +# Serialise the entire build/deploy/test cycle: no other run can move staging tags. +concurrency: + group: staging-release + cancel-in-progress: false + env: REGISTRY: privaterepo.sitaru.org BACKEND_IMAGE_NAME: ${{ gitea.repository }}-backend @@ -12,7 +17,18 @@ env: PIPELINE_IMAGE_NAME: ${{ gitea.repository }}-pipeline jobs: + prepare: + runs-on: ubuntu-latest + outputs: + build_id: ${{ steps.identity.outputs.build_id }} + steps: + - id: identity + run: python3 -c 'import uuid; print("build_id=" + uuid.uuid4().hex)' >> "$GITHUB_OUTPUT" + build-backend: + needs: [prepare] + outputs: + digest: ${{ steps.build.outputs.digest }} name: Build Backend (FastAPI) runs-on: ubuntu-latest steps: @@ -46,17 +62,24 @@ jobs: type=raw,value=staging - name: Build and push Backend Docker image + id: build uses: docker/build-push-action@v5 with: context: . file: ./Dockerfile push: true + build-args: | + BUILD_SHA=${{ gitea.sha }} + BUILD_ID=${{ needs.prepare.outputs.build_id }} tags: ${{ steps.meta-backend.outputs.tags }} labels: ${{ steps.meta-backend.outputs.labels }} cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.BACKEND_IMAGE_NAME }}:buildcache cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.BACKEND_IMAGE_NAME }}:buildcache,mode=max build-frontend: + needs: [prepare] + outputs: + digest: ${{ steps.build.outputs.digest }} name: Build Frontend (Next.js) runs-on: ubuntu-latest steps: @@ -90,18 +113,23 @@ jobs: type=raw,value=staging - name: Build and push Frontend Docker image + id: build uses: docker/build-push-action@v5 with: context: ./nextjs-app file: ./nextjs-app/Dockerfile push: true + build-args: | + BUILD_SHA=${{ gitea.sha }} + BUILD_ID=${{ needs.prepare.outputs.build_id }} tags: ${{ steps.meta-frontend.outputs.tags }} labels: ${{ steps.meta-frontend.outputs.labels }} - build-args: | - FASTAPI_URL=http://backend:80/api # Cache disabled due to registry size limits build-pipeline: + needs: [prepare] + outputs: + digest: ${{ steps.build.outputs.digest }} name: Build Pipeline (Meltano + dbt + Airflow) runs-on: ubuntu-latest steps: @@ -135,11 +163,15 @@ jobs: type=raw,value=staging - name: Build and push Pipeline Docker image + id: build uses: docker/build-push-action@v5 with: context: ./pipeline file: ./pipeline/Dockerfile push: true + build-args: | + BUILD_SHA=${{ gitea.sha }} + BUILD_ID=${{ needs.prepare.outputs.build_id }} tags: ${{ steps.meta-pipeline.outputs.tags }} labels: ${{ steps.meta-pipeline.outputs.labels }} cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.PIPELINE_IMAGE_NAME }}:buildcache @@ -148,30 +180,23 @@ jobs: deploy-staging: name: Deploy to Staging runs-on: ubuntu-latest - needs: [build-backend, build-frontend, build-pipeline] + needs: [prepare, build-backend, build-frontend, build-pipeline] steps: - name: Trigger staging stack update run: curl -fsSk -X POST "${{ secrets.PORTAINER_STAGING_WEBHOOK }}" - - name: Wait for staging to become healthy - run: | - echo "Polling ${STAGING_BASE_URL} for up to 5 minutes..." - for i in $(seq 1 60); do - if curl -fsS -o /dev/null --max-time 10 "${STAGING_BASE_URL}/"; then - echo "Staging is up (attempt $i)" - exit 0 - fi - sleep 5 - done - echo "Staging did not become healthy in time" >&2 - exit 1 + - uses: actions/checkout@v4 + - name: Verify deployed release identity + run: python3 scripts/ci/release.py wait env: - STAGING_BASE_URL: ${{ secrets.STAGING_BASE_URL }} + BASE_URL: ${{ secrets.STAGING_BASE_URL }} + EXPECTED_SHA: ${{ gitea.sha }} + EXPECTED_BUILD_ID: ${{ needs.prepare.outputs.build_id }} e2e-staging: name: E2E Journeys against Staging runs-on: ubuntu-latest - needs: [deploy-staging] + needs: [prepare, deploy-staging, build-backend, build-frontend, build-pipeline] steps: - name: Checkout repository uses: actions/checkout@v4 @@ -187,11 +212,42 @@ jobs: npm ci npx playwright install --with-deps chromium + - name: Verify release before journeys + run: python3 scripts/ci/release.py wait --timeout 10 + env: + BASE_URL: ${{ secrets.STAGING_BASE_URL }} + EXPECTED_SHA: ${{ gitea.sha }} + EXPECTED_BUILD_ID: ${{ needs.prepare.outputs.build_id }} + - name: Run E2E journeys working-directory: e2e run: npx playwright test env: BASE_URL: ${{ secrets.STAGING_BASE_URL }} + EXPECTED_SHA: ${{ gitea.sha }} + EXPECTED_BUILD_ID: ${{ needs.prepare.outputs.build_id }} + + - name: Verify release after journeys + run: python3 scripts/ci/release.py wait --timeout 10 + env: + BASE_URL: ${{ secrets.STAGING_BASE_URL }} + EXPECTED_SHA: ${{ gitea.sha }} + EXPECTED_BUILD_ID: ${{ needs.prepare.outputs.build_id }} + + - uses: docker/setup-buildx-action@v3 + - uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ gitea.actor }} + password: ${{ secrets.REGISTRY_TOKEN }} + - name: Mark tested image digests as verified + run: python3 scripts/ci/release.py verify + env: + EXPECTED_SHA: ${{ gitea.sha }} + EXPECTED_BUILD_ID: ${{ needs.prepare.outputs.build_id }} + BACKEND_DIGEST: ${{ needs.build-backend.outputs.digest }} + FRONTEND_DIGEST: ${{ needs.build-frontend.outputs.digest }} + PIPELINE_DIGEST: ${{ needs.build-pipeline.outputs.digest }} # Production deployment is a second, manual approval: see promote.yml # ("Promote to Production (manual)") and docs/DEPLOY.md. diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index b7d3a11..09a843d 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -68,13 +68,13 @@ jobs: python-version: "3.12" - name: Install dependencies - run: pip install -r requirements.txt pytest "httpx<0.28" + run: pip install -r requirements.txt pytest "httpx<0.28" pyyaml - name: Import smoke test run: python -c "from backend.app import app; print('backend imports OK')" - name: Backend unit tests - run: python -m pytest backend/tests -q + run: python -m pytest backend/tests pipeline/tests scripts/ci/tests -q build-backend: name: Build Backend (no push) diff --git a/.gitea/workflows/promote.yml b/.gitea/workflows/promote.yml index 777530c..35652bb 100644 --- a/.gitea/workflows/promote.yml +++ b/.gitea/workflows/promote.yml @@ -97,33 +97,15 @@ jobs: username: ${{ gitea.actor }} password: ${{ secrets.REGISTRY_TOKEN }} - - name: Retag approved images as prod (keeping rollback pointer) - run: | - SHORT_SHA="${{ steps.resolve.outputs.short }}" - for IMAGE in \ - "${REGISTRY}/${BACKEND_IMAGE_NAME}" \ - "${REGISTRY}/${FRONTEND_IMAGE_NAME}" \ - "${REGISTRY}/${PIPELINE_IMAGE_NAME}"; do - # Keep a rollback pointer before moving :prod - docker buildx imagetools create -t "${IMAGE}:prod-previous" "${IMAGE}:prod" || true - docker buildx imagetools create -t "${IMAGE}:prod" "${IMAGE}:${SHORT_SHA}" - echo "Promoted ${IMAGE}:${SHORT_SHA} -> :prod" - done + - name: Resolve verified digests and promote the complete image set + run: python3 scripts/ci/release.py promote --output release.json + env: + EXPECTED_SHA: ${{ steps.resolve.outputs.full }} - name: Trigger production stack update run: curl -fsSk -X POST "${{ secrets.PORTAINER_PROD_WEBHOOK }}" - - name: Wait for production to become healthy - run: | - echo "Polling ${PROD_BASE_URL} for up to 5 minutes..." - for i in $(seq 1 60); do - if curl -fsS -o /dev/null --max-time 10 "${PROD_BASE_URL}/"; then - echo "Production is up (attempt $i)" - exit 0 - fi - sleep 5 - done - echo "Production did not become healthy in time" >&2 - exit 1 + - name: Verify production release identity + run: python3 scripts/ci/release.py wait --release release.json env: - PROD_BASE_URL: ${{ secrets.PROD_BASE_URL }} + BASE_URL: ${{ secrets.PROD_BASE_URL }} diff --git a/Dockerfile b/Dockerfile index 6b9011f..c048914 100644 --- a/Dockerfile +++ b/Dockerfile @@ -24,6 +24,12 @@ RUN pip install --no-cache-dir -r requirements.txt COPY backend/ ./backend/ COPY scripts/ ./scripts/ +ARG BUILD_SHA=development +ARG BUILD_ID=development +LABEL io.schoolcompare.build-id=$BUILD_ID +LABEL io.schoolcompare.commit=$BUILD_SHA +RUN python -c 'import json,sys; open("backend/build-info.json", "w").write(json.dumps({"sha":sys.argv[1],"build_id":sys.argv[2]}))' "$BUILD_SHA" "$BUILD_ID" + # Expose the application port EXPOSE 80 diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4ea42cd..a00995e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -80,9 +80,11 @@ API types. `payload-types.ts` and the Payload import map are generated artifacts 1. Airflow DAGs extract and validate source data, then run selected dbt builds. 2. Relevant DAGs rebuild Typesense and swap the `schools` alias. -3. They call `POST /api/admin/reload` with `X-API-Key` to refresh school DataFrames. -4. A separate weekly sitemap DAG calls `POST /api/admin/regenerate-sitemap`, - rebuilding places and sitemaps. +3. They call `POST /api/admin/reload` with `X-API-Key`. It builds and validates + replacement DataFrames, places, reverse membership and sitemaps off the request + loop, then publishes them together. Failure returns 503 and preserves live data. +4. A separate weekly sitemap DAG can regenerate the derived publication from the + current DataFrame without clearing the live registry first. GIAS is scheduled daily, Ofsted monthly, and annual datasets are manually triggered. The DAG definitions are authoritative for selectors and dependencies. @@ -92,16 +94,25 @@ backend HTTP Cache-Control/ETags, Next.js fetch/page revalidation, and browser o shared HTTP caches where configured. Place fetches request a one-week revalidation interval. HTTP ETags are computed after route execution, not before database work. -Known limitations: reload clears the old DataFrames before verifying replacement -data; places/sitemaps refresh separately; Next.js caches are not explicitly purged -by the pipeline; Typesense import results are not validated before alias publication. -Do not describe this sequence as an atomic dataset release. These are follow-up -reliability tasks, not changes implemented by the documentation cleanup. +Typesense publication validates every import response and the final document +count before switching aliases. A session-scoped PostgreSQL advisory lock +serialises index reads/publication across DAGs. The previous collection remains +available for rollback; old unaliased collections are pruned after success. +Failed drafts are retained until a later successful cleanup, because an uncertain +alias-update response must never cause deletion of a potentially live index. + +The backend snapshot swap is process-local and assumes the current single-worker +deployment. It is not an atomic transaction spanning PostgreSQL marts, Typesense +and Next.js caches. Next.js caches are not explicitly purged by the pipeline. +School search retrieves every Typesense candidate before applying API filters; +only a dependency failure invokes substring fallback, not a valid empty match set. ## Deployment references See [DEPLOY.md](DEPLOY.md). PR checks include frontend typechecking/tests, backend unit tests, image builds and AI review. Staging journeys run after merging. -Production promotion retags a selected commit's images. Current health polling -checks HTTP success, not the deployed commit identity; overlapping staging runs -remain a release-verification concern. +Staging runs are serialised across builds, deployment and E2E. Build-stamped +frontend/backend identities are checked before and after journeys. Only then are +the captured image digests marked verified. Promotion resolves and validates the +complete verified image set before retagging production. See the runbook for +first-rollout requirements and remaining integration checks. diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 2cff1b8..63e85e0 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -19,8 +19,9 @@ PR checks (.gitea/workflows/pr-checks.yml) ▼ Stage pipeline (.gitea/workflows/deploy.yml) — automatic 1. build & push images → tags sha-, staging - 2. staging Portainer webhook → wait for staging health + 2. staging Portainer webhook → verify frontend/backend SHA + build ID 3. Playwright E2E journeys against staging ← gate before human testing + 4. verify identity again; tag tested digests verified- ▼ Manual testing on staging (stx.schoolcompare.co.uk) │ Actions → "Promote to Production (manual)" ← approval #2 @@ -28,14 +29,15 @@ Manual testing on staging (stx.schoolcompare.co.uk) Promote pipeline (.gitea/workflows/promote.yml) — manual dispatch 1. resolve target sha (input, or latest main if empty) 2. REFUSE unless that commit's "E2E Journeys against Staging" status is green - 3. retag sha- → :prod (same bytes — build once, promote the image) + 3. resolve verified- digests, validate labels, retag digests → :prod previous :prod saved as :prod-previous - 4. prod Portainer webhook → wait for prod health + 4. prod Portainer webhook → verify expected SHA + build ID ``` Key principle: **build once, promote the exact image**. Production pins `:prod`, which only moves when a human runs the promote workflow — and the workflow -only accepts commits that passed the staging E2E gate. Nothing tags `:latest` +only accepts commits that passed the staging E2E gate and have a complete verified +image set. Nothing tags `:latest` anymore. ## Branch & PR workflow @@ -256,3 +258,41 @@ how long any feature is exposed to this. If `UNLEASH_URL` is unset, every flag is `False` and no connection is attempted. That is the correct behaviour for local development and CI, and it means the test suites need no flag server. + +## Release identity and the P1 reliability gate + +Every staging run creates a random build ID before building its three images. +Each image carries the commit and build ID as labels. Frontend/backend images +also contain a build-time JSON file; environment overrides cannot rewrite it. +`/release.json` returns both identities with `Cache-Control: no-store`. It fails +with 503 when either identity cannot be read. FastAPI's internal endpoint is +`/api/release`. + +The entire staging workflow shares one concurrency group, with cancellation +disabled. This needs Gitea 1.26 or newer, where workflow concurrency is supported +([release notes](https://blog.gitea.com/release-of-1.26.0/)); the configured server +reported 1.27.3 during this change. Do not run the workflow on an older server +that ignores the concurrency key. Manual deployments outside this workflow must +also avoid changing staging during journeys. + +The gate checks both identities before and after Playwright. It then validates +labels on the captured build output digests and tags them `verified-`. +The manual promotion script resolves all three verified tags to immutable digests +and confirms one matching commit/build ID before moving any `:prod` tag. It polls +production for that same identity using a locally saved release manifest. +A registry error can still interrupt the three tag writes; the Portainer webhook +only runs after successful promotion, and rerunning promotion resolves the full +verified set again. There is no cross-registry atomic tag transaction. + +**First rollout:** old green commits without verified tags/build identities are +not promotable through this gate. Build and test a commit containing the new +workflow first. The release route must be reachable through the configured +`STAGING_BASE_URL`/`PROD_BASE_URL`; it deliberately avoids the public staging +`/api` proxy limitation. No new deployment secret is required. + +`scripts/ci/release.py` implements identity polling and digest verification. +Its mocked tests run in PR checks alongside backend and index-publication tests. +The new Playwright journeys also check deployed identity and stale pagination. +Local unit checks do not validate registry credentials, Portainer behaviour, +proxy routing or a deployed image; those require the staging run. Production +promotion remains a separate human action. diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index f1ebb0c..613cc85 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -42,8 +42,8 @@ From the repository root, using an available Python 3.11 or 3.12 interpreter: ```sh python3.11 -m venv /tmp/schoolcompare-backend-venv -/tmp/schoolcompare-backend-venv/bin/python -m pip install -r requirements.txt pytest 'httpx<0.28' -/tmp/schoolcompare-backend-venv/bin/python -m pytest backend/tests -q +/tmp/schoolcompare-backend-venv/bin/python -m pip install -r requirements.txt pytest 'httpx<0.28' pyyaml +/tmp/schoolcompare-backend-venv/bin/python -m pytest backend/tests pipeline/tests scripts/ci/tests -q ``` Substitute `python3.12` if matching PR CI. The test dependencies above match the diff --git a/e2e/tests/reliability.spec.ts b/e2e/tests/reliability.spec.ts new file mode 100644 index 0000000..31a2d4a --- /dev/null +++ b/e2e/tests/reliability.spec.ts @@ -0,0 +1,41 @@ +import { test, expect, Route } from '@playwright/test'; + +test('the deployed frontend and backend report the tested build', async ({ request }) => { + const response = await request.get('/release.json'); + expect(response.ok()).toBeTruthy(); + expect(response.headers()['cache-control']).toContain('no-store'); + const identity = await response.json(); + expect(identity.frontend).toEqual(identity.backend); + expect(identity.frontend.sha).toMatch(/^[a-f0-9]{40}$/); + expect(identity.frontend.build_id).toMatch(/^[a-f0-9]{32}$/); + if (process.env.EXPECTED_SHA) expect(identity.frontend.sha).toBe(process.env.EXPECTED_SHA); + if (process.env.EXPECTED_BUILD_ID) expect(identity.frontend.build_id).toBe(process.env.EXPECTED_BUILD_ID); +}); + +test('changing search while loading another page does not append old results', async ({ page }) => { + await page.goto('/?phase=primary'); + await expect(page.getByRole('button', { name: 'Load more schools' })).toBeVisible(); + let received!: (route: Route) => void; + const pending = new Promise(resolve => { received = resolve; }); + await page.route('**/api/schools?**', async route => { + if (new URL(route.request().url()).searchParams.get('page') === '2') { + received(route); + return; + } + await route.continue(); + }); + await page.getByRole('button', { name: 'Load more schools' }).click(); + const oldRequest = await pending; + const search = page.getByPlaceholder('School name or postcode').first(); + await search.fill('secondary'); + await search.press('Enter'); + await page.waitForURL(/search=secondary/); + // A cancelled fetch may prevent route fulfilment altogether; either way, + // this deliberately late response must not become part of the new results. + await oldRequest.fulfill({ json: { + schools: [{ urn: 999998, school_name: 'P1 stale result sentinel', phase: 'Primary' }], + total: 2, page: 2, page_size: 1, total_pages: 2, + } }).catch(() => {}); + await expect(page.getByText('P1 stale result sentinel')).toHaveCount(0); + await expect(page.getByRole('button', { name: 'Loading...' })).toHaveCount(0); +}); diff --git a/nextjs-app/Dockerfile b/nextjs-app/Dockerfile index af446c9..21962b5 100644 --- a/nextjs-app/Dockerfile +++ b/nextjs-app/Dockerfile @@ -28,6 +28,10 @@ ENV NODE_ENV=production ARG FASTAPI_URL=http://backend:80/api ENV FASTAPI_URL=${FASTAPI_URL} +ARG BUILD_SHA=development +ARG BUILD_ID=development +RUN node -e 'require("fs").writeFileSync("build-info.json", JSON.stringify({sha:process.argv[1],build_id:process.argv[2]}))' "$BUILD_SHA" "$BUILD_ID" + # Build application RUN npm run build @@ -70,6 +74,12 @@ USER nextjs EXPOSE 3000 # Set environment variables +ARG BUILD_SHA=development +ARG BUILD_ID=development +LABEL io.schoolcompare.build-id=$BUILD_ID +LABEL io.schoolcompare.commit=$BUILD_SHA +COPY --from=builder /app/build-info.json ./build-info.json + ENV PORT=3000 ENV HOSTNAME="0.0.0.0" diff --git a/nextjs-app/__tests__/app/releaseIdentity.test.ts b/nextjs-app/__tests__/app/releaseIdentity.test.ts new file mode 100644 index 0000000..a2476d1 --- /dev/null +++ b/nextjs-app/__tests__/app/releaseIdentity.test.ts @@ -0,0 +1,30 @@ +/** @jest-environment node */ +import { GET } from '@/app/(frontend)/release.json/route'; +import { readFile } from 'node:fs/promises'; + +jest.mock('node:fs/promises', () => ({ readFile: jest.fn() })); +const realFetch = global.fetch; +const identity = { sha: 'a'.repeat(40), build_id: 'b'.repeat(32) }; +beforeEach(() => { + jest.mocked(readFile).mockResolvedValue(JSON.stringify(identity)); + global.fetch = jest.fn(async () => Response.json(identity)); +}); +afterEach(() => { global.fetch = realFetch; jest.resetAllMocks(); }); + +test('reports immutable file identity and backend identity without caching', async () => { + const response = await GET(); + expect(response.status).toBe(200); + expect(response.headers.get('Cache-Control')).toBe('no-store'); + expect(await response.json()).toEqual({ frontend: identity, backend: identity }); + expect(fetch).toHaveBeenCalledWith(expect.stringMatching(/\/api\/release$/), expect.objectContaining({ cache: 'no-store', signal: expect.anything() })); +}); + +test('missing build metadata cannot pass the release gate', async () => { + jest.mocked(readFile).mockRejectedValueOnce(new Error('missing file')); + expect((await GET()).status).toBe(503); +}); + +test('backend failure cannot pass the release gate', async () => { + jest.mocked(fetch).mockResolvedValueOnce(new Response('', { status: 503 })); + expect((await GET()).status).toBe(503); +}); diff --git a/nextjs-app/app/(frontend)/release.json/route.ts b/nextjs-app/app/(frontend)/release.json/route.ts new file mode 100644 index 0000000..9dd7456 --- /dev/null +++ b/nextjs-app/app/(frontend)/release.json/route.ts @@ -0,0 +1,17 @@ +import { readFile } from 'node:fs/promises'; +import path from 'node:path'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +export async function GET() { + try { + const frontend = JSON.parse(await readFile(path.join(process.cwd(), 'build-info.json'), 'utf8')); + const base = process.env.FASTAPI_URL || 'http://localhost:8000/api'; + const res = await fetch(`${base}/release`, { cache: 'no-store', signal: AbortSignal.timeout(5000) }); + if (!res.ok) throw new Error('Backend identity unavailable'); + return Response.json({ frontend, backend: await res.json() }, { headers: { 'Cache-Control': 'no-store', 'X-Robots-Tag': 'noindex' } }); + } catch { + return Response.json({ detail: 'Release identity unavailable' }, { status: 503, headers: { 'Cache-Control': 'no-store', 'X-Robots-Tag': 'noindex' } }); + } +} diff --git a/pipeline/Dockerfile b/pipeline/Dockerfile index cf16a25..68599d3 100644 --- a/pipeline/Dockerfile +++ b/pipeline/Dockerfile @@ -40,4 +40,9 @@ ENV AIRFLOW_HOME=/opt/airflow ENV AIRFLOW__CORE__DAGS_FOLDER=/opt/pipeline/dags ENV PYTHONPATH=/opt/pipeline +ARG BUILD_SHA=development +ARG BUILD_ID=development +LABEL io.schoolcompare.build-id=$BUILD_ID +LABEL io.schoolcompare.commit=$BUILD_SHA + CMD ["airflow", "api-server"] diff --git a/scripts/ci/release.py b/scripts/ci/release.py new file mode 100644 index 0000000..fd41c62 --- /dev/null +++ b/scripts/ci/release.py @@ -0,0 +1,136 @@ +"""Release identity checks and promotion of the exact digests that passed E2E. + +Uses only the standard library and Docker Buildx. No registry mutation happens +until every image in the set has been resolved and its build labels validated. +""" +import argparse +import json +import os +from pathlib import Path +import re +import subprocess +import time +from urllib.request import Request, urlopen + +COMPONENTS = ('BACKEND', 'FRONTEND', 'PIPELINE') + + +def docker(*args): + return subprocess.check_output(['docker', 'buildx', 'imagetools', *args], text=True).strip() + + +def check_sha(sha): + if not re.fullmatch(r'[0-9a-f]{40}', sha): + raise ValueError('Expected a full commit SHA') + return sha + + +def check_digest(digest): + if not re.fullmatch(r'sha256:[0-9a-f]{64}', digest): + raise ValueError('Expected an immutable image digest') + return digest + + +def image_identity(ref): + image = json.loads(docker('inspect', ref, '--format', '{{json .Image}}')) + configs = [image] if 'config' in image else list(image.values()) + identities = set() + for config in configs: + labels = config['config']['Labels'] + identities.add((labels['io.schoolcompare.commit'], labels['io.schoolcompare.build-id'])) + if len(identities) != 1: + raise ValueError('Image platforms disagree about their release identity') + return next(iter(identities)) + + +def resolve_images(sha, verified=False, expected_build_id=None): + check_sha(sha) + refs = [] + build_ids = set() + for component in COMPONENTS: + image = f"{os.environ['REGISTRY']}/{os.environ[component + '_IMAGE_NAME']}" + if verified: + manifest = json.loads(docker('inspect', f'{image}:verified-{sha}', '--format', '{{json .Manifest}}')) + digest = check_digest(manifest['digest']) + else: + digest = check_digest(os.environ[component + '_DIGEST']) + ref = f'{image}@{digest}' + actual_sha, build_id = image_identity(ref) + if actual_sha != sha or not re.fullmatch(r'[0-9a-f]{32}', build_id): + raise ValueError(f'Unrecognised release identity for {component}') + if expected_build_id is not None and build_id != expected_build_id: + raise ValueError(f'Build identity mismatch for {component}') + build_ids.add(build_id) + refs.append((image, ref)) + if len(build_ids) != 1: + raise ValueError('Refusing a mixed image set') + return {'sha': sha, 'build_id': build_ids.pop(), 'images': refs} + + +def verify(sha, build_id): + release = resolve_images(sha, expected_build_id=build_id) + for image, ref in release['images']: + docker('create', '-t', f'{image}:verified-{sha}', ref) + return release + + +def promote(sha): + release = resolve_images(sha, verified=True) + # Resolve all targets first; never discover a missing candidate halfway through. + for image, _ in release['images']: + try: + docker('create', '-t', f'{image}:prod-previous', f'{image}:prod') + except subprocess.CalledProcessError: + print(f'No rollback pointer saved for {image}', flush=True) + for image, ref in release['images']: + docker('create', '-t', f'{image}:prod', ref) + return release + + +def matches(payload, sha, build_id): + return all(payload.get(component) == {'sha': sha, 'build_id': build_id} + for component in ('frontend', 'backend')) + + +def wait(base_url, sha, build_id, timeout): + check_sha(sha) + if not re.fullmatch(r'[0-9a-f]{32}', build_id): + raise ValueError('Missing expected build identity') + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + try: + req = Request(f'{base_url.rstrip("/")}/release.json?check={time.time_ns()}', + headers={'Cache-Control': 'no-cache'}) + with urlopen(req, timeout=min(10, max(.1, deadline - time.monotonic()))) as response: + payload = json.load(response) + if matches(payload, sha, build_id): + print(f'Verified deployed release {sha} / {build_id}') + return + except (OSError, ValueError): + pass + time.sleep(min(5, max(0, deadline - time.monotonic()))) + raise RuntimeError('Deployment did not report the expected frontend/backend release') + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('action', choices=['wait', 'verify', 'promote']) + parser.add_argument('--timeout', type=float, default=300) + parser.add_argument('--release', type=Path) + parser.add_argument('--output', type=Path) + args = parser.parse_args() + identity = json.loads(args.release.read_text()) if args.release else { + 'sha': os.environ.get('EXPECTED_SHA', ''), + 'build_id': os.environ.get('EXPECTED_BUILD_ID', ''), + } + if args.action == 'wait': + wait(os.environ['BASE_URL'], identity['sha'], identity['build_id'], args.timeout) + return + result = (verify(identity['sha'], identity['build_id']) if args.action == 'verify' + else promote(identity['sha'])) + if args.output: + args.output.write_text(json.dumps(result)) + + +if __name__ == '__main__': + main() diff --git a/scripts/ci/tests/test_release.py b/scripts/ci/tests/test_release.py new file mode 100644 index 0000000..c98af86 --- /dev/null +++ b/scripts/ci/tests/test_release.py @@ -0,0 +1,65 @@ +import json +from unittest.mock import Mock +import pytest +from scripts.ci import release + +SHA = 'a' * 40 +BUILD = 'b' * 32 +DIGESTS = ['sha256:' + c * 64 for c in '123'] + + +@pytest.fixture +def docker(monkeypatch): + monkeypatch.setenv('REGISTRY', 'registry.example') + refs = {} + for component, digest in zip(release.COMPONENTS, DIGESTS): + monkeypatch.setenv(component + '_IMAGE_NAME', component.lower()) + monkeypatch.setenv(component + '_DIGEST', digest) + refs[f'registry.example/{component.lower()}'] = digest + def run(*args): + if args[0] == 'create': return '' + if args[-1] == '{{json .Manifest}}': + return json.dumps({'digest': refs[args[1].split(':')[0]]}) + return json.dumps({'config': {'Labels': {'io.schoolcompare.commit': SHA, + 'io.schoolcompare.build-id': BUILD}}}) + mock = Mock(side_effect=run) + monkeypatch.setattr(release, 'docker', mock) + return mock + + +def test_wrong_deployed_build_is_rejected_even_at_same_commit(): + assert not release.matches({'frontend': {'sha': SHA, 'build_id': BUILD}, + 'backend': {'sha': SHA, 'build_id': 'c' * 32}}, SHA, BUILD) + assert release.matches({c: {'sha': SHA, 'build_id': BUILD} for c in ('frontend', 'backend')}, SHA, BUILD) + + +def test_verification_tags_the_captured_digests(docker): + release.verify(SHA, BUILD) + creates = [c.args for c in docker.call_args_list if c.args[0] == 'create'] + assert len(creates) == 3 + for call, digest in zip(creates, DIGESTS): + assert call[-1].endswith('@' + digest) + assert call[2].endswith(':verified-' + SHA) + + +def test_promotion_resolves_all_verified_images_before_mutation(docker): + result = release.promote(SHA) + assert result['build_id'] == BUILD + calls = [c.args for c in docker.call_args_list] + first_write = next(i for i, c in enumerate(calls) if c[0] == 'create') + assert first_write == 6 # each of three candidates needs manifest + config + assert all(c[-1].endswith('@' + d) for c, d in zip(calls[-3:], DIGESTS)) + + +def test_mixed_builds_fail_before_any_tag_is_changed(docker, monkeypatch): + identities = iter([(SHA, BUILD), (SHA, 'c' * 32), (SHA, BUILD)]) + monkeypatch.setattr(release, 'image_identity', lambda _: next(identities)) + with pytest.raises(ValueError, match='mixed'): + release.promote(SHA) + assert not any(c.args[0] == 'create' for c in docker.call_args_list) + + +def test_missing_candidate_fails_before_any_tag_is_changed(docker): + docker.side_effect = RuntimeError('missing verified tag') + with pytest.raises(RuntimeError): release.promote(SHA) + assert not any(c.args[0] == 'create' for c in docker.call_args_list) diff --git a/scripts/ci/tests/test_workflows.py b/scripts/ci/tests/test_workflows.py new file mode 100644 index 0000000..adb3bae --- /dev/null +++ b/scripts/ci/tests/test_workflows.py @@ -0,0 +1,34 @@ +"""Check the dependency graph that ties tested digests to deployable images.""" +from pathlib import Path +import yaml + +ROOT = Path(__file__).resolve().parents[3] + + +def test_staging_verifies_identity_before_and_after_journeys(): + workflow = yaml.safe_load((ROOT / '.gitea/workflows/deploy.yml').read_text()) + assert workflow['concurrency'] == {'group': 'staging-release', 'cancel-in-progress': False} + jobs = workflow['jobs'] + for component in ('backend', 'frontend', 'pipeline'): + job = jobs['build-' + component] + assert 'prepare' in job['needs'] + assert job['outputs']['digest'] == '${{ steps.build.outputs.digest }}' + build = next(step for step in job['steps'] if step.get('id') == 'build') + assert 'BUILD_ID=${{ needs.prepare.outputs.build_id }}' in build['with']['build-args'] + steps = jobs['e2e-staging']['steps'] + runs = [step.get('run', '') for step in steps] + test = runs.index('npx playwright test') + assert 'release.py wait' in runs[test - 1] + assert 'release.py wait' in runs[test + 1] + assert 'release.py verify' in runs[-1] + for component in ('backend', 'frontend', 'pipeline'): + assert 'build-' + component in jobs['e2e-staging']['needs'] + assert component.upper() + '_DIGEST' in steps[-1]['env'] + + +def test_promotion_uses_verified_digest_resolver_and_build_identity_poll(): + workflow = yaml.safe_load((ROOT / '.gitea/workflows/promote.yml').read_text()) + steps = workflow['jobs']['promote-prod']['steps'] + runs = [step.get('run', '') for step in steps] + assert 'python3 scripts/ci/release.py promote --output release.json' in runs + assert runs[-1] == 'python3 scripts/ci/release.py wait --release release.json' -- 2.54.0 From 5ad1cbfb537dead49cff6b7627e733e131a1ae2f Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 15 Sep 2026 11:21:45 +0100 Subject: [PATCH 5/5] fix(api): bound the search candidate set instead of draining Typesense MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fetching every match kept scoped searches correct but left the number of round trips in the caller's hands: a one-letter query, or a deliberately broad one, walked the whole collection a page at a time. Cap the candidate set at 1,000 URNs — four pages — and return the relevance-ordered prefix when the ceiling is hit. That is still far more than one page, so the API's own authority, phase and postcode filters keep the matches they need, while latency and upstream load stay bounded. A capped query is logged so a genuinely truncated search is visible. Co-Authored-By: Claude Opus 5 --- backend/data_loader.py | 39 ++++++++++++++++----- backend/tests/test_search_completeness.py | 41 +++++++++++++++++++++++ docs/ARCHITECTURE.md | 6 ++-- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/backend/data_loader.py b/backend/data_loader.py index 1816745..79ba386 100644 --- a/backend/data_loader.py +++ b/backend/data_loader.py @@ -84,34 +84,55 @@ def _get_typesense_client(): return None -def search_schools_typesense(query: str) -> Optional[List[int]]: - """Return all matching URNs in relevance order; None means unavailable. +SEARCH_PAGE_SIZE = 250 +# Search results are filtered again by the API (authority, phase, postcode, +# etc.), so one page is too small for scoped searches. Keep the candidate set +# bounded, though: a broad query must not turn into an unbounded sequence of +# Typesense requests. Four pages is enough to preserve useful scoped matches +# while putting a hard ceiling on latency and upstream load. +SEARCH_MAX_CANDIDATES = 1_000 - Filtering and user pagination happen in the API after this search. Returning - only the first search page would silently discard valid local matches. - Never return a partial candidate set if a later page fails. + +def search_schools_typesense(query: str) -> Optional[List[int]]: + """Return a bounded set of matching URNs in relevance order. + + ``None`` means Typesense is unavailable; ``[]`` is a valid zero-match + result. The API applies its remaining filters after this search, so the + first few pages are fetched rather than only the first page. Once the + candidate ceiling is reached, the relevance-ordered prefix is returned on + purpose; fetching every match would make common or adversarial queries + unbounded. """ client = _get_typesense_client() if client is None: return None - urns = [] + urns: list[int] = [] + fetched = 0 try: page = 1 - while True: + while fetched < SEARCH_MAX_CANDIDATES: + page_size = min(SEARCH_PAGE_SIZE, SEARCH_MAX_CANDIDATES - fetched) result = client.collections["schools"].documents.search({ "q": query, "query_by": "school_name,local_authority,postcode", - "per_page": 250, + "per_page": page_size, "page": page, "typo_tokens_threshold": 1, }) hits = result.get("hits", []) urns.extend(int(h["document"]["urn"]) for h in hits) - if len(urns) >= result.get("found", len(urns)): + fetched += len(hits) + if fetched >= result.get("found", fetched): return list(dict.fromkeys(urns)) if not hits: raise ValueError("Search pagination ended before all matches arrived") page += 1 + logging.getLogger(__name__).info( + "Typesense search capped at %d candidates for query %r", + SEARCH_MAX_CANDIDATES, + query, + ) + return list(dict.fromkeys(urns)) except Exception: logging.getLogger(__name__).exception("School search unavailable") return None diff --git a/backend/tests/test_search_completeness.py b/backend/tests/test_search_completeness.py index e170a8b..b32634a 100644 --- a/backend/tests/test_search_completeness.py +++ b/backend/tests/test_search_completeness.py @@ -23,6 +23,47 @@ def test_search_returns_matches_beyond_first_page(monkeypatch): assert pages == [1, 2] +def test_search_caps_broad_queries_at_a_bounded_number_of_pages(monkeypatch): + requests = [] + + def search(params): + requests.append(params) + return { + 'found': 10_000, + 'hits': [ + {'document': {'urn': 100000 + params['page'] * 1000 + i}} + for i in range(params['per_page']) + ], + } + + client_for(monkeypatch, search) + result = data_loader.search_schools_typesense('school') + + assert len(result) == data_loader.SEARCH_MAX_CANDIDATES + assert len(requests) == data_loader.SEARCH_MAX_CANDIDATES // data_loader.SEARCH_PAGE_SIZE + assert all(request['per_page'] == data_loader.SEARCH_PAGE_SIZE for request in requests) + assert requests[-1]['page'] == len(requests) + + +def test_search_uses_a_smaller_final_page_when_the_cap_is_not_a_page_multiple(monkeypatch): + monkeypatch.setattr(data_loader, 'SEARCH_MAX_CANDIDATES', 251) + requests = [] + + def search(params): + requests.append(params) + return { + 'found': 10_000, + 'hits': [{'document': {'urn': 100000 + len(requests) * 1000 + i}} + for i in range(params['per_page'])], + } + + client_for(monkeypatch, search) + result = data_loader.search_schools_typesense('school') + + assert len(result) == 251 + assert [request['per_page'] for request in requests] == [250, 1] + + def test_later_page_failure_does_not_return_partial_results(monkeypatch): def search(params): if params['page'] == 2: diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a00995e..a94087d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -104,8 +104,10 @@ alias-update response must never cause deletion of a potentially live index. The backend snapshot swap is process-local and assumes the current single-worker deployment. It is not an atomic transaction spanning PostgreSQL marts, Typesense and Next.js caches. Next.js caches are not explicitly purged by the pipeline. -School search retrieves every Typesense candidate before applying API filters; -only a dependency failure invokes substring fallback, not a valid empty match set. +School search retrieves a relevance-ordered candidate prefix (currently capped at +1,000 URNs) before applying API filters. This keeps scoped searches useful while +putting a hard ceiling on Typesense round trips; only a dependency failure invokes +substring fallback, not a valid empty match set. ## Deployment references -- 2.54.0