diff --git a/changelog.d/962.changed.md b/changelog.d/962.changed.md new file mode 100644 index 000000000..d2762abca --- /dev/null +++ b/changelog.d/962.changed.md @@ -0,0 +1,3 @@ +The gateway no longer records an exception, which `policyengine-observability` writes at ERROR severity, for a request it answers with a 400 or 404 client error: a `ValueError` from route resolution on `POST /simulate/economy/comparison` or `POST /simulate/economy/budget-window`, such as the unknown country sent by the authentication test that each deployment runs, an unknown job ID on `GET /jobs/{job_id}`, an unknown batch ID on `GET /budget-window-jobs/{batch_job_id}`, or an unknown kind on `GET /versions/{kind}`. Status codes and response bodies are unchanged, and each request's `request.completed` record still reports the response with outcome `client_error`. Server errors, and the 404 for a job whose gateway metadata exists but whose Modal call is missing or whose output has expired, still record their exceptions. + +On both economy submission routes, a `ValueError` or `HuggingFaceDatasetReferenceError` without an SPM error code, raised while describing the route's bundle or resolving its SPM settings, now returns 500 instead of 400. Only bad data in the deployed bundle manifest can raise it. The gateway logs the exception with its traceback and records it as a handled server error, and the response has `status: "failed"` and the redacted error `Simulation failed (correlation_id=...)` instead of the exception text. A coded SPM error still returns 400 with structured `errors`. Both routes declare the 500 in the gateway's OpenAPI document and in the entrypoint's, so the generated client's two submission functions handle it as a declared status. diff --git a/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/app.py b/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/app.py index be3a699ce..759610fc6 100644 --- a/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/app.py +++ b/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/app.py @@ -329,6 +329,9 @@ def _register_simulation_routes( "description": "Invalid request or SPM selection", "model": SimulationErrorResponse, }, + 500: { + "description": "Server failure (redacted error with a correlation ID)" + }, }, dependencies=[Depends(submitter)], ) @@ -368,6 +371,9 @@ async def submit_comparison( "description": "Invalid request or SPM selection", "model": SimulationErrorResponse, }, + 500: { + "description": "Server failure (redacted error with a correlation ID)" + }, }, dependencies=[Depends(submitter)], ) diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py index f61c2cf03..374019dd3 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py @@ -40,6 +40,9 @@ def create_openapi_app() -> FastAPI: responses={ 200: {"description": "Job submitted successfully"}, 400: {"description": "Invalid request (unknown country/version)"}, + 500: { + "description": "Server failure (redacted error with a correlation ID)" + }, }, ) async def submit_simulation(request: SimulationRequest) -> JobSubmitResponse: @@ -56,6 +59,9 @@ async def submit_simulation(request: SimulationRequest) -> JobSubmitResponse: responses={ 200: {"description": "Budget-window batch submitted successfully"}, 400: {"description": "Invalid request (unknown country/version/year)"}, + 500: { + "description": "Server failure (redacted error with a correlation ID)" + }, }, ) async def submit_budget_window_batch( diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_creation_routes.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_creation_routes.py index 36bbc4ed3..2ca0cbc9c 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_creation_routes.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_creation_routes.py @@ -35,6 +35,7 @@ resolve_spm_selection, spm_error_detail, ) +from policyengine_simulation_observability.errors import log_and_redact_exception from policyengine_simulation_observability.identifiers import ( OBSERVABILITY_ID_HEADER, resolve_observability_id, @@ -226,7 +227,6 @@ async def submit_simulation( ) _require_route_serves_breakdown(request, route) except ValueError as e: - runtime.record_exception(e, handled=True, status_code=400) raise HTTPException(status_code=400, detail=str(e)) from e runtime.set_context( @@ -259,8 +259,20 @@ async def submit_simulation( errors=[detail.model_dump()], headers=observability_headers(observability_id), ) - runtime.record_exception(exc, handled=True, status_code=400) - raise HTTPException(status_code=400, detail=str(exc)) from exc + # No request input reaches this point: the country and version are + # already checked, and every request-driven SPM error has a code. The + # error comes from the deployed bundle manifest, so it is a server + # failure, and the caller gets a redacted message. + redacted_error = log_and_redact_exception( + exc, + runtime=runtime, + scope="simulation_submission", + context={"resolved_app_name": route.app_name}, + ) + return failed_job_response( + error=redacted_error, + headers=observability_headers(observability_id), + ) if request.spm is not None: payload["spm"] = request.spm.model_dump(mode="json") @@ -336,7 +348,6 @@ async def submit_budget_window_batch( request.policyengine_version, ) except ValueError as e: - runtime.record_exception(e, handled=True, status_code=400) raise HTTPException(status_code=400, detail=str(e)) from e runtime.set_context( @@ -360,8 +371,20 @@ async def submit_budget_window_batch( errors=[detail.model_dump()], headers=observability_headers(observability_id), ) - runtime.record_exception(exc, handled=True, status_code=400) - raise HTTPException(status_code=400, detail=str(exc)) from exc + # No request input reaches this point: the country and version are + # already checked, and every request-driven SPM error has a code. The + # error comes from the deployed bundle manifest, so it is a server + # failure, and the caller gets a redacted message. + redacted_error = log_and_redact_exception( + exc, + runtime=runtime, + scope="budget_window_submission", + context={"resolved_app_name": route.app_name}, + ) + return failed_job_response( + error=redacted_error, + headers=observability_headers(observability_id), + ) with runtime.span(BUDGET_WINDOW_STAGES.name(Stage.REQUEST_PARSE)): payload = _build_budget_window_parent_payload( request, diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_status_routes.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_status_routes.py index 986440248..daf0cec25 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_status_routes.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/job_status_routes.py @@ -46,7 +46,6 @@ logger, observability_headers, observability_runtime, - record_not_found, router, ) @@ -122,7 +121,6 @@ async def get_job_status( with runtime.span(ANNUAL_IMPACT_STAGES.name(Stage.MODAL_JOB_METADATA_READ)): job_metadata = job_metadata_store().get(job_id) if job_metadata is None: - record_not_found(runtime, f"Job not found: {job_id}") raise HTTPException(status_code=404, detail=f"Job not found: {job_id}") observability_id = bind_observability_id( request, @@ -226,7 +224,6 @@ async def get_budget_window_job_status( with runtime.span(BUDGET_WINDOW_STAGES.name(Stage.BUDGET_WINDOW_STATE_LOAD)): seed_state = get_batch_job_seed(batch_job_id) if seed_state is None: - record_not_found(runtime, f"Budget-window job not found: {batch_job_id}") raise HTTPException( status_code=404, detail=f"Budget-window job not found: {batch_job_id}" ) diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/routes_common.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/routes_common.py index 7bd809b03..140ea2d0d 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/routes_common.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/routes_common.py @@ -25,11 +25,6 @@ def observability_runtime(request: Request) -> ObservabilityRuntime: return runtime -def record_not_found(runtime: ObservabilityRuntime, message: str) -> None: - """Record a handled not-found lookup (status 404) on the request's telemetry.""" - runtime.record_exception(LookupError(message), handled=True, status_code=404) - - def bind_observability_id( http_request: Request, runtime: ObservabilityRuntime, diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/version_routes.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/version_routes.py index 5dc1e8c5d..44f4e3f44 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/version_routes.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/version_routes.py @@ -29,7 +29,6 @@ from policyengine_simulation_gateway.routes_common import ( logger, observability_runtime, - record_not_found, router, ) @@ -92,7 +91,6 @@ async def get_country_versions(kind: str, request: Request) -> VersionMap: runtime = observability_runtime(request) kind_lower = kind.lower() if kind_lower not in SUPPORTED_ROUTE_KINDS: - record_not_found(runtime, f"Unknown version kind: {kind}") raise HTTPException(status_code=404, detail=f"Unknown version kind: {kind}") with runtime.span(ANNUAL_IMPACT_STAGES.name(Stage.ROUTE_RESOLUTION)): diff --git a/projects/policyengine-simulation-gateway/tests/golden/openapi.json b/projects/policyengine-simulation-gateway/tests/golden/openapi.json index 9a8e102ea..ae9c9d9c5 100644 --- a/projects/policyengine-simulation-gateway/tests/golden/openapi.json +++ b/projects/policyengine-simulation-gateway/tests/golden/openapi.json @@ -35,6 +35,9 @@ "400": { "description": "Invalid request (unknown country/version)" }, + "500": { + "description": "Server failure (redacted error with a correlation ID)" + }, "422": { "description": "Validation Error", "content": { @@ -77,6 +80,9 @@ "400": { "description": "Invalid request (unknown country/version/year)" }, + "500": { + "description": "Server failure (redacted error with a correlation ID)" + }, "422": { "description": "Validation Error", "content": { diff --git a/projects/policyengine-simulation-gateway/tests/test_budget_window_endpoints.py b/projects/policyengine-simulation-gateway/tests/test_budget_window_endpoints.py index 875eb478c..36ccf4261 100644 --- a/projects/policyengine-simulation-gateway/tests/test_budget_window_endpoints.py +++ b/projects/policyengine-simulation-gateway/tests/test_budget_window_endpoints.py @@ -135,7 +135,7 @@ def test__given_budget_window_submission__then_returns_parent_batch_job_id( } @pytest.mark.usefixtures("mock_modal") - def test__given_unknown_budget_window_job__then_records_handled_404( + def test__given_unknown_budget_window_job__then_404_without_exception( self, client: TestClient, monkeypatch ): recorded_errors = [] @@ -151,13 +151,7 @@ def test__given_unknown_budget_window_job__then_records_handled_404( assert response.json()["detail"] == ( "Budget-window job not found: missing-batch" ) - assert str(recorded_errors[0][0]) == ( - "Budget-window job not found: missing-batch" - ) - assert recorded_errors[0][1] == { - "handled": True, - "status_code": 404, - } + assert recorded_errors == [] def test__given_parent_lookup_failure__then_degraded_poll_is_not_an_error( self, mock_modal, client: TestClient, monkeypatch diff --git a/projects/policyengine-simulation-gateway/tests/test_endpoints.py b/projects/policyengine-simulation-gateway/tests/test_endpoints.py index 1e4bf293b..1c4b9f872 100644 --- a/projects/policyengine-simulation-gateway/tests/test_endpoints.py +++ b/projects/policyengine-simulation-gateway/tests/test_endpoints.py @@ -226,7 +226,7 @@ def test__given_active_state__then_policyengine_versions_endpoint_works( assert response.json()["latest"] == "4.10.0" @pytest.mark.usefixtures("mock_modal") - def test__given_unknown_version_kind__then_records_handled_404( + def test__given_unknown_version_kind__then_404_without_exception( self, client: TestClient, monkeypatch ): recorded_errors = [] @@ -240,11 +240,7 @@ def test__given_unknown_version_kind__then_records_handled_404( assert response.status_code == 404 assert response.json()["detail"] == "Unknown version kind: fr" - assert str(recorded_errors[0][0]) == "Unknown version kind: fr" - assert recorded_errors[0][1] == { - "handled": True, - "status_code": 404, - } + assert recorded_errors == [] def test__given_no_active_state__then_versions_fall_back_to_old_dicts( self, mock_modal, client: TestClient diff --git a/projects/policyengine-simulation-gateway/tests/test_exception_records.py b/projects/policyengine-simulation-gateway/tests/test_exception_records.py new file mode 100644 index 000000000..52ed4248b --- /dev/null +++ b/projects/policyengine-simulation-gateway/tests/test_exception_records.py @@ -0,0 +1,158 @@ +"""Client errors the gateway answers with 4xx record no exception; 5xx still do. + +``ObservabilityRuntime.record_exception`` always writes an ERROR record. A +request the gateway answers with a 4xx already gets a ``request.completed`` +record whose outcome is ``client_error``, so an exception record for it adds +only a false alarm to the daily error report. A server failure (5xx) keeps +its exception record. + +An uncoded error while describing the route's bundle comes from the deployed +bundle manifest, not from the request, so it is a server failure. +""" + +import re +from typing import NamedTuple + +import pytest +from fastapi.testclient import TestClient +from policyengine_simulation_contract.hf_dataset import HuggingFaceDatasetReferenceError +from policyengine_simulation_gateway import job_creation_routes +from policyengine_simulation_observability.identifiers import OBSERVABILITY_ID_HEADER + +# The country the deploy pipeline's authentication test sends, expecting 400. +AUTHENTICATION_PROBE_COUNTRY = "__authentication_probe__" + + +class Submission(NamedTuple): + """A submission endpoint and the request fields it needs besides country.""" + + path: str + request_fields: dict[str, object] + + +SUBMISSIONS = [ + pytest.param( + Submission( + path="/simulate/economy/comparison", + request_fields={"scope": "macro", "reform": {}}, + ), + id="comparison", + ), + pytest.param( + Submission( + path="/simulate/economy/budget-window", + request_fields={ + "region": "us", + "scope": "macro", + "reform": {}, + "start_year": "2026", + "window_size": 2, + }, + ), + id="budget_window", + ), +] + +RecordedException = tuple[BaseException, dict[str, object]] + + +@pytest.fixture +def recorded_exceptions( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> list[RecordedException]: + """Replace the runtime's ``record_exception`` with one that keeps each call.""" + recorded: list[RecordedException] = [] + + def record_exception(error: BaseException, **keyword_arguments: object) -> None: + recorded.append((error, keyword_arguments)) + + monkeypatch.setattr( + client.app.state.policyengine_observability, + "record_exception", + record_exception, + ) + return recorded + + +@pytest.mark.parametrize("submission", SUBMISSIONS) +def test__given_unknown_country__then_returns_400_without_exception_record( + mock_modal, + client: TestClient, + recorded_exceptions: list[RecordedException], + submission: Submission, +): + response = client.post( + submission.path, + json={"country": AUTHENTICATION_PROBE_COUNTRY, **submission.request_fields}, + ) + + assert response.status_code == 400 + assert response.json() == { + "detail": f"Unknown country: {AUTHENTICATION_PROBE_COUNTRY}" + } + assert recorded_exceptions == [] + assert mock_modal["func"].calls == [] + + +@pytest.mark.parametrize("submission", SUBMISSIONS) +def test__given_bundle_lookup_error__then_records_it_and_returns_redacted_500( + mock_modal, + client: TestClient, + recorded_exceptions: list[RecordedException], + monkeypatch: pytest.MonkeyPatch, + submission: Submission, +): + malformed_dataset_reference = "hf://policyengine/internal-repo/missing.h5" + manifest_error = HuggingFaceDatasetReferenceError( + f"Malformed dataset reference in bundle manifest: {malformed_dataset_reference}" + ) + + def reject_bundle_lookup(*_arguments: object) -> None: + raise manifest_error + + monkeypatch.setattr( + job_creation_routes, "build_policyengine_bundle", reject_bundle_lookup + ) + + response = client.post( + submission.path, json={"country": "us", **submission.request_fields} + ) + + assert response.status_code == 500 + body = response.json() + assert body["status"] == "failed" + assert body["result"] is None + assert "errors" not in body + assert re.fullmatch( + r"Simulation failed \(correlation_id=[0-9a-f]{32}\)", body["error"] + ) + assert malformed_dataset_reference not in response.text + assert OBSERVABILITY_ID_HEADER in response.headers + assert recorded_exceptions == [ + (manifest_error, {"handled": True, "status_code": 500}), + ] + assert mock_modal["func"].calls == [] + + +def test__given_modal_lookup_failure__then_job_status_records_server_exception( + mock_modal, + client: TestClient, + recorded_exceptions: list[RecordedException], +): + submit_response = client.post( + "/simulate/economy/comparison", + json={"country": "us", "scope": "macro", "reform": {}}, + ) + job_id = submit_response.json()["job_id"] + lookup_failure = RuntimeError("Modal control plane unavailable") + mock_modal["function_call"].from_id_errors[job_id] = lookup_failure + client_receiving_server_errors = TestClient( + client.app, raise_server_exceptions=False + ) + + response = client_receiving_server_errors.get(f"/jobs/{job_id}") + + assert response.status_code == 500 + assert recorded_exceptions == [ + (lookup_failure, {"handled": False, "status_code": 500}), + ] diff --git a/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py b/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py index 0f972c26f..627748b50 100644 --- a/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py +++ b/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py @@ -79,14 +79,15 @@ def test__given_submitted_job_with_header__then_polling_echoes_header( assert response.headers[OBSERVABILITY_ID_HEADER] == OBSERVABILITY_ID @pytest.mark.usefixtures("mock_modal") - def test__given_unknown_job_id__then_polling_returns_404( + def test__given_unknown_job_id__then_polling_returns_404_without_exception( self, client: TestClient, monkeypatch ): """Return 404 for a job ID the gateway never issued, without asking Modal. Given a job id that the gateway never issued When polling job status - Then the gateway returns 404 before asking Modal for a call result. + Then the gateway returns 404 before asking Modal for a call result, + and records no exception, because the 404 is a client error. """ recorded_errors = [] monkeypatch.setattr( @@ -103,11 +104,7 @@ def test__given_unknown_job_id__then_polling_returns_404( assert response.status_code == 404 assert response.json()["detail"] == "Job not found: unknown-job-id" assert OBSERVABILITY_ID_HEADER not in response.headers - assert str(recorded_errors[0][0]) == "Job not found: unknown-job-id" - assert recorded_errors[0][1] == { - "handled": True, - "status_code": 404, - } + assert recorded_errors == [] def test__given_lazy_modal_call_without_metadata__then_polling_returns_404( self, mock_modal, client: TestClient