Skip to content

Harden VA-Spec annotation exports against shape-dependent failure #797

Description

@bencap

Summary

A shape-dependent failure in annotation construction currently reaches users as an uninformative network error, truncates NDJSON streams without signalling, and is detected only when someone reports it. Make export failures visible and attributable, stop one bad variant from destroying a stream, verify the corpus, and pin the shapes that surface as regression cases.

Problem

Four gaps on one path.

Unhandled exceptions lose their response. @app.exception_handler(Exception) at src/mavedb/server_main.py:206 returns a 500 with {"message": "Internal server error"}. Starlette passes Exception handlers to ServerErrorMiddleware, outside the user middleware stack, so the response never passes through CORSMiddleware (server_main.py:85) and carries no Access-Control-Allow-Origin. The browser rejects it and axios reports Network Error. Verified on starlette 0.49.3 against this app's middleware order: a route returning normally gets ACAO: *, a route raising gets a 500 with no ACAO. The body also uses message where every other error path uses detail, and carries no correlation id, so a reported failure cannot be tied to its log line.

A single variant aborts a whole stream. _stream_generated_annotations (src/mavedb/routers/score_sets.py:1207) catches only MappingDataDoesntExistException. Anything else raised after the first yield truncates a response that already sent 200, and the consumer cannot distinguish the result from a complete file. Raising paths that are not MappingDataDoesntExistException:

  • lib/annotation/study_result.py:24variant.data["score_data"]["score"], KeyError or TypeError on absent or malformed score data
  • lib/annotation/util.py:72ValueError on an unrecognized VRS Allele state type
  • lib/annotation/util.py:434 — indexing of metadata-derived identifiers

Nothing checks the corpus. Unit tests cannot have full-database access, so no artifact answers whether annotation construction succeeds for every record stored. 8412508 is the precedent: allele_from_mapped_variant_dictionary_result unconditionally constructed a LiteralSequenceExpression, producing a 500 for reference-identical variants whose state is a ReferenceLengthExpression. That shape existed in the data and in no test.

Success paths run on one shape. The three /va/ routes and tests/lib/annotation/test_annotate.py exercise a single variant shape, and test_annotate.py contains no model_validate of an emitted object, so a structural regression in emitted output passes every test in it.

Proposed behavior

  1. Produce the unhandled-exception 500 from middleware inside the user stack so CORSMiddleware wraps it, with {"detail": ..., "correlation_id": ...} as the body. Preserve the existing logging and Slack alert.
  2. Catch Exception per record in _stream_generated_annotations, emit {"variant_urn": ..., "annotation": null, "error": {"type": ..., "detail": ...}}, continue, and close with a summary record carrying counts of emitted, null, and errored records.
  3. mavedb.scripts.export_sweep — walk published score sets, attempt each of the three annotation types, and write (urn, surface, outcome, exception_class, message) rows, where outcome is one of ok, exception, schema_violation. --sample N bounds per-variant work.
  4. A round-trip assertion — model_dump(exclude_none=True) serialized to JSON, re-parsed through model_validate, equal to the original — used both as the sweep's schema check and across a parametrized shape list in test_annotate.py.

Acceptance criteria

  • A route raising an uncaught exception returns 500 carrying Access-Control-Allow-Origin for an allowed origin, with a body containing detail and the correlation id. The failure is still logged and still alerts Slack.
  • A stream in which one variant raises returns 200, emits one record per variant, marks the failing record with error, and never truncates. Line count equals X-Total-Count plus one summary record.
  • ui/src/composables/use-score-set-downloads.ts tolerates records carrying error and reports the failed count to the user.
  • The sweep emits one row per attempted (score set, surface) including successes, does not stop on failure, states the sample size used, and exits non-zero if any row is not ok.
  • Run against data predating 8412508, the sweep reports a non-ok row for at least one reference-identical variant.
  • Every (annotation function, shape) pair either round-trips through its GA4GH model or returns None. No pair raises an uncaught exception.
  • The shape list includes a reference-identical variant with a ReferenceLengthExpression state, and covers nucleotide-level assay, protein-level assay, indel, synonymous or no-change, non-coding target, no gnomAD record, and no ClinVar control.
  • Adding a shape is one entry in the shape list, not a new test function per route.

Implementation notes

  • Middleware is registered at server_main.py:77-93; the last add_middleware call is outermost. Reuse format_raised_exception_info_as_dict (lib/logging/context.py:75) and log_request (lib/logging/canonical.py:67). Both export routers already set route_class=LoggedRoute, so the correlation id is already in the logs and only missing from the body.
  • UI consumer is ui/src/composables/use-score-set-downloads.ts:111. parse_ndjson_response (tests/helpers/util/common.py:41) is the test-side parser.
  • The sweep runs against a restored database snapshot rather than test fixtures. Obtaining and restoring one is an operator step outside this issue. scripts/environment.py provides script_environment and with_database_session, the pattern export_public_data.py uses.
  • create_mock_mapped_variant (tests/helpers/mocks/factories.py:311) hardcodes pre_mapped and post_mapped. Adding kwargs that default to the current constants makes the VRS payload constants already in tests/helpers/constants.py usable as the shape list with no new construction machinery: TEST_VALID_POST_MAPPED_VRS_ALLELE_RLE, ..._LENGTH_EXPRESSION, ..._CIS_PHASED_BLOCK, ..._VRS1_X, ..._VRS2_X, TEST_MAPPED_VARIANT_WITH_HGVS_G_EXPRESSION, TEST_MAPPED_VARIANT_WITH_HGVS_P_EXPRESSION.
  • Models: ga4gh.va_spec.base.core.ExperimentalVariantFunctionalImpactStudyResult, ga4gh.va_spec.base.core.Statement, ga4gh.va_spec.acmg_2015.VariantPathogenicityStatement. The mavedb_calibration_scope extension is emitted unconditionally and must survive the round trip.
  • Shapes the sweep surfaces are added to the shape list as regression cases.

Metadata

Metadata

Assignees

Labels

app: backendTask implementation touches the backend

Type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions