Repository navigation
fix(api): render NaN as null and keep CORS headers on unhandled 500s - #157
Merged
Merged
Conversation
Collaborator
Author
CORs errors present when accessing the specific run |
GET /verify/compose-api-prod-comparison-212-... returned a 500 that the browser reported as a CORS error. COPASI left one report variable NaN, so its self-comparison score was NaN; the completed report was stored and queried fine, but Starlette's JSONResponse renders with allow_nan=False and raised "Out of range float values are not JSON compliant: nan". That unhandled exception was rendered by ServerErrorMiddleware, outside CORSMiddleware, so the 500 had no Access-Control-Allow-Origin header. - NonFiniteSafeJSONResponse (app default_response_class) renders NaN/+-Infinity as null, which the frontend already handles. Only content the strict encoder rejects is re-walked. - ServerErrorCorsMiddleware renders unhandled exceptions as a JSON 500 inside CORS, then re-raises so the server still logs the traceback. - Revert the hardcoded biosim/api.biosim origins: biosim-gke already allows the frontend via CORS_EXTRA_ORIGINS, and CORS was not the cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /verify/omex and /verify/runs accepted rel_tol, abs_tol_min and abs_tol_scale of nan/inf from the query string. Such a request started a workflow whose every score is NaN and spent a rate-limit unit and a simulator job per requested simulator on a run that could never produce a result. Before the NaN-safe response class, it also made every GET for that verification (and the POST's own response) a 500. allow_inf_nan=False rejects these before any upload, ledger write or workflow start. Finite values are unchanged; the OpenAPI spec is unchanged (regenerated, no diff). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…serialization Added functionality to the NonFiniteSafeJSONResponse class to detect and raise a ValueError for circular references during JSON serialization, preventing RecursionError. Introduced helper functions to manage non-finite values and circular references effectively. Updated tests to verify correct behavior for circular content and shared references.
…tation plans for fixes Added a detailed summary of the root cause for CORS errors and 500 responses on the verify page, confirming that the CORS issue was a symptom of an underlying problem with NaN values in reports. Documented decisions regarding future fixes, including stopping frontend polling on errors and adding request IDs to 500 responses. Updated revision notes to reflect these changes and the implementation steps for the proposed fixes.
Contributor
|
the NaN and infinity issues in JSON (and the use of NaN as a flag rather than an error condition) has been a thorn in my side for years. Maybe it is okay to push this through right now, but we'll have to revisit it in the future. In some projects I have used custom serialization/deserialization to marshall these special values (NaN, +inf, -inf). push it through now, but I consider this tech debt. for instance, why was there NaN or Inf in the first place, this should have been the result of a bad result. I am a bit strict about these things, because otherwise you end up tripping over these problems much futher down the line, or ignoring them completely. |
…iation details Revised the implementation plan to reflect the reconciliation of the verification 500/CORS fix with PR #157. Clarified the status of the fix, updated evidence labels, and documented the changes made in the diff, including the handling of NaN values and CORS headers. Added details on future plans and the files affected by the changes.
Restructure the plan into a durable incident record: summary and decisions, impact and timeline, root cause, where the NaN and Inf values come from (truncated COPASI output, the upstream "nan" string that Pydantic coerces, and calc_stats), the four fixes, PR #157's changes and tests, deployment, technical debt from the review, prioritized follow-ups, open questions, a diagnosis runbook, and evidence. Corrects the earlier draft: a +Inf or a partial NaN in calc_stats input produces a false match rather than being harmless, and negative tolerances make every variable match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Opening certain completed verification runs failed in the browser with a CORS error. Behind each one was an HTTP 500 from
GET /verify/{workflow_id}. CORS itself was configured correctly. The API crashed while rendering a stored report that containedNaN, and Starlette sent that crash response from outsideCORSMiddleware, so it had no CORS headers.This PR:
NaNand±Infinityas JSONnullin any response, instead of failing it.backend/docs/verify-500-cors-fix-implementation-plan.md.The full write-up covers the root cause, where the NaN comes from, the four fixes, technical debt and follow-ups, plus a diagnosis runbook. It is in that document.
Root cause
Confirmed from production logs and data:
compose-api-prod-comparison-212-…(BIOMD0000000155), COPASI 4.45.296 returned only the first output step of the time course: time0.0, 0.01, where PySCeS ran to10.0in 1001 points. Seven of its 18 variables were NaN at that step, and one was NaN at both points."nan", and Pydantic converts it to a float NaN without warning.calc_statsthen scored the all-NaN variable asnan, even when comparing COPASI with itself. Temporal stored the report as is.GET /verifyread the report, and Starlette'sJSONResponse(json.dumps(allow_nan=False)) raised:ServerErrorMiddleware, which sits outsideCORSMiddleware. The 500 had noAccess-Control-Allow-Origin, so the browser reported a CORS error and hid the status.COMPLETED, because the final status is recorded before the response is rendered. The dialog showedPENDINGand polled forever.Scope: six verifications produced 127 of these 500s on 2026-10-22 and 2026-10-23, 85 of them for run 212. Each one later became a normal
404 … no longer retainedonce its Temporal history expired, about a day after completion.Changes
backend/biosim_server/common/json_response.py(new)NonFiniteSafeJSONResponseis the app'sdefault_response_class. It renders NaN and ±Infinity asnull, which is also what Pydantic'smodel_dump_jsonproduces.ValueError.ValueError("Circular reference detected")instead of recursing intoRecursionError, and content shared without a cycle is kept.The frontend already handles
nullin these fields:formatScoreshows—, the analytics composables use?? null, and anullin chart data is drawn as a gap.backend/biosim_server/common/server_errors.py(new)ServerErrorCorsMiddlewareis registered insideCORSMiddleware, the same pattern as the existing 413 fromMultipartBodyLimitMiddleware. It renders an unhandled exception as{"detail": "Internal Server Error"}with status 500, then re-raises it, asServerErrorMiddlewaredoes:@app.exception_handler(Exception)would not work here: Starlette installs that handler onServerErrorMiddleware, which is outside CORS.backend/biosim_server/api/main.pyrel_tol,abs_tol_minandabs_tol_scaleonPOST /verify/omexandPOST /verify/runsnow useallow_inf_nan=False.?rel_tol=nanstarted a workflow whose every score was NaN, at the cost of a rate-limit unit and one simulator job per simulator.https://biosim.biosimulations.organdhttps://api.biosim.biosimulations.orgorigins added in this branch's first commit.biosim-gkealready allows the frontend throughCORS_EXTRA_ORIGINS, and requests from the API host to itself are same-origin.APP_ORIGINSand inbackend/CLAUDE.md.Other
backend/docs/verify-500-cors-fix-implementation-plan.md: the investigation record described above.backend/CLAUDE.md: a note in the CORS section on why error responses must stay insideCORSMiddleware, and that NaN renders asnull..gitignore: ignores the machine-local.perch/directory.Tests
backend/tests/common/test_json_response.py, 8 tests:score, NaN insim_run_datavalues) returns 200 fromGET /verify/{id}, withnulls and CORS headers.nullat any depth.TypeError.ValueErrorinstead ofRecursionError, and content shared without a cycle is kept.backend/tests/common/test_server_errors.py, 3 tests:access-control-allow-originand doesn't leak the exception message.backend/tests/api/test_verification_completion.py, 18 cases:nan,infor-infin any tolerance, on either POST endpoint, returns 422 naming the parameter. There is no upload, no ledger call and no workflow start.The regression tests fail without the fix:
The NaN report test fails with the production
ValueError … nan.The 500 test fails on the missing CORS header.
All 18 tolerance cases fail.
pytest: the full backend suite passes (backend-ci: 1547 passed, 17 skipped)ruff checkandmypy biosim_server testsare cleanCI is green (
backend-ci,frontend-ci,smoke, GitGuardian, Snyk)Deployment
Merging changes nothing in production on its own. Releasing it takes:
backend-v0.10.3)biosim-gkedeploy PR, followingbackend/CLAUDE.md→ DeployThe release also includes the unreleased verify-workflow change from #126 (
17e213a). That change doesn't alter the Temporal command sequence, so no worker drain is needed.Technical debt and follow-ups (not in this PR)
The review of this PR accepted it as an immediate fix and recorded the handling of non-finite values as technical debt: NaN and Inf should be treated as the sign of a bad result, not passed along as a value. The investigation supports that view. Full detail and priorities are in §8–§9 of the document.
High:
calc_statsreports false matches on non-finite input.+Infmakes a variable's tolerance infinite, so[100, inf, -50]vs[1, 2, 3]scores 0 and counts as a match.Non-finite input should become an explicit "not comparable" error. How to represent that should be agreed with reviewers first.
High: Fix 3, frontend. The run dialog should stop polling after repeated failures, including failures with no status, and show the error instead of
PENDING. The plan is in §5.4.Medium:
nullfallback is used."nan"conversion.Low / deferred:
scoreandvaluesaslist[float | None]in the schema.🤖 Generated with Claude Code