From 16a532bd3d1606e58ba50404c53be484627db0a0 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:07:29 -0500 Subject: [PATCH] Discriminate InvalidScanFileError's cause with a closed, non-PII enum The capture-scan sweep retries a refused ingest every 30 seconds forever, and a future change needs to decide when to stop. It cannot key that decision on InvalidScanFileError alone: the nine raise sites in ingest_scan.handler span causes as different as a half-transferred file (transient, retrying helps once the writer finishes) and a structurally incomplete scan with no rotation-angle dataset (permanent, no retry will ever produce one). Catching the exception class collapses that distinction, and misclassifying a transient cause as permanent means silently abandoning a file that would have ingested fine later. ScanFileInvalidReason gives each of the nine sites its own member, with is_transient as the machine-readable split, cited per member from the port docstrings that already document it (ScanReader's "may be transient" language, ChecksumVerifier's Unreachable, and so on) rather than from intuition. Values are plain PascalCase labels that can never embed a path, so they stay safe to log and persist even though the messages they're discriminated from carry personal 2-BM directory paths. No sites were merged: every pair considered (the two captured_at causes, the two locator causes, Unreadable vs DigestUnreachable) has a distinct remedy or failure stage. InvalidScanFileError.reason carries the new enum alongside the unchanged message; str(exc) is untouched, so this is purely additive at the HTTP boundary. The only behavioral use added is the capture scan ingestor's warning log, which now names the reason an operator can act on without opening the HDF5 file by hand. Co-Authored-By: Claude Sonnet 5 --- .../src/cora/api/_capture_scan_ingestor.py | 15 ++- apps/api/src/cora/data/errors.py | 111 ++++++++++++++++++ .../cora/data/features/ingest_scan/handler.py | 29 +++-- .../unit/api/test_capture_scan_ingestor.py | 46 +++++++- apps/api/tests/unit/data/test_errors.py | 74 ++++++++++++ .../unit/data/test_ingest_scan_handler.py | 36 +++++- 6 files changed, 288 insertions(+), 23 deletions(-) create mode 100644 apps/api/tests/unit/data/test_errors.py diff --git a/apps/api/src/cora/api/_capture_scan_ingestor.py b/apps/api/src/cora/api/_capture_scan_ingestor.py index 07aabb2d944..f4460be1261 100644 --- a/apps/api/src/cora/api/_capture_scan_ingestor.py +++ b/apps/api/src/cora/api/_capture_scan_ingestor.py @@ -101,7 +101,9 @@ `{UserLastName}-{ProposalNumber}`; see `run.aggregates.run.capture_path`'s module docstring) and this log sink is not the vault: it cannot be erased. Every log line here carries -`run_id` and `capture_code` only, never the path and never an +`run_id` and `capture_code` only (the `invalid_scan_file` line also +carries `reason`, the closed `ScanFileInvalidReason` enum, which by +construction can never embed a path), never the path and never an exception's rendered message (`InvalidScanFileError`'s text embeds the locator via `repr()`), mirroring `_run_translator.py`'s identical rule for the same value. @@ -436,16 +438,17 @@ async def _ingest_one(self, candidate: ScanIngestCandidate) -> _Outcome: existing_dataset_id=str(exc.existing_dataset_id), ) return _Outcome.SKIP - except InvalidScanFileError: + except InvalidScanFileError as exc: # Never `str(exc)`: the message embeds the locator via - # `repr()` (see this module's own docstring). The class - # alone already says "structurally incomplete", "unreadable", - # or "no timestamp"; that's enough for an operator to act on - # without the path. + # `repr()` (see this module's own docstring). `exc.reason` + # is the closed, non-PII `ScanFileInvalidReason` enum, safe + # to log because it can never carry a path; that's enough + # for an operator to act on without the path. _log.warning( "capture_scan_ingestor.invalid_scan_file", capture_code=candidate.capture_code, run_id=str(candidate.run_id), + reason=exc.reason.value, ) return _Outcome.SKIP except ( diff --git a/apps/api/src/cora/data/errors.py b/apps/api/src/cora/data/errors.py index eef2daf7e16..64bd7fba63a 100644 --- a/apps/api/src/cora/data/errors.py +++ b/apps/api/src/cora/data/errors.py @@ -12,6 +12,106 @@ "BC-application-layer errors"). """ +from enum import StrEnum + + +class ScanFileInvalidReason(StrEnum): + """Why `InvalidScanFileError` refused a scan file, discriminated from + the message text so a caller can branch on cause without parsing a + string or ever touching the path the message may (redacted or not) + embed. + + One member per raise site in `ingest_scan.handler`; none merged. + Every pair considered kept a real diagnostic distinction: the two + `captured_at` members have opposite remedies (drop the supplied + value versus supply one); `UNREADABLE` and `DIGEST_UNREACHABLE` are + distinct pipeline stages behind distinct ports, and collapsing them + would lose which pass failed; `LOCATOR_UNRESOLVED` and + `LOCATOR_MISSING_FILENAME` fail for different reasons at different + call sites. + + `is_transient` is the machine-readable permanent/transient split: + true when retrying the SAME command unmodified could plausibly + succeed once the file's writer finishes; false when nothing about + the file changing can help, because the fix requires the caller to + change the request, the file, or the configuration. Each + classification below is cited from the code that justifies it, not + from intuition. + + - `LOCATOR_UNRESOLVED` (permanent): `resolve_capture_path_locator` + returns `None` only for "malformed locator; no vault row at + all; a row that exists but not at the location the locator + names; filename mismatch" (its own docstring). The sweep's + candidate query only selects vault rows that already exist, so + a failure here is drift or misconfiguration, not a file still + arriving. + - `UNREADABLE` (transient): `ScanReader`'s port docstring: "may be + transient: a half-transferred file on the analysis tier is an + expected observable, and the caller decides whether to retry." + - `UNRECOGNIZED` (permanent): the same `ScanReader` docstring + pointedly withholds the "may be transient" language from this + variant; it fires only when the layout's one mandatory dataset + is entirely absent, a layout verdict rather than an I/O timing + issue. + - `STRUCTURALLY_INCOMPLETE` (permanent): `DataExchangeScanReader` + documents the absent rotation-angle dataset as meaning + post-processing "has not completed (not yet run, failed + permanently... or a scan that produced zero projections)". The + sweep only ever considers terminal runs (`status IN + ('Completed', 'Aborted')`), which narrows but does not fully + eliminate the "not yet run" sub-case this member bundles + together with the two permanent ones; whoever builds the + finality-axis work the port docstring defers should revisit + this member first. + - `DIGEST_UNREACHABLE` (transient): reuses `ChecksumVerifier`'s + `Unreachable`, whose docstring says outright "(transient)". + - `CHANGED_WHILE_READING` (transient): fires only when size or + mtime moved between two stat snapshots taken moments apart, + which by construction means a writer touched the file during + the read window. + - `CAPTURED_AT_AMBIGUOUS` (permanent): the remedy is to drop the + supplied value; the CALLER's request must change, not the file. + - `CAPTURED_AT_MISSING` (permanent): the remedy requires an + operator to supply `captured_at` manually. `CaptureScanIngestor` + (the only automated caller) never passes `captured_at`, so this + can never self-resolve through automated retry. + - `LOCATOR_MISSING_FILENAME` (permanent): the locator names a + directory, not a file; a caller/configuration defect, not a + file-readiness one. + + Member name SCREAMING_SNAKE; string value PascalCase, matching + `AttestationKind`'s house style. Values are plain enum labels only + and must never embed, interpolate, or derive from a filesystem + path, so they stay safe to log and persist forever. + """ + + LOCATOR_UNRESOLVED = "LocatorUnresolved" + UNREADABLE = "Unreadable" + UNRECOGNIZED = "Unrecognized" + STRUCTURALLY_INCOMPLETE = "StructurallyIncomplete" + DIGEST_UNREACHABLE = "DigestUnreachable" + CHANGED_WHILE_READING = "ChangedWhileReading" + CAPTURED_AT_AMBIGUOUS = "CapturedAtAmbiguous" + CAPTURED_AT_MISSING = "CapturedAtMissing" + LOCATOR_MISSING_FILENAME = "LocatorMissingFilename" + + @property + def is_transient(self) -> bool: + """See the class docstring for the per-member citation backing + this split; kept as a property (not a bare frozenset check + inline at each call site) so the transient/permanent verdict + has exactly one place to change.""" + return self in _TRANSIENT_SCAN_FILE_INVALID_REASONS + + +_TRANSIENT_SCAN_FILE_INVALID_REASONS = frozenset( + { + ScanFileInvalidReason.UNREADABLE, + ScanFileInvalidReason.DIGEST_UNREACHABLE, + ScanFileInvalidReason.CHANGED_WHILE_READING, + } +) + class InvalidScanFileError(ValueError): """The file at the ingest locator cannot be ingested as commanded. @@ -28,10 +128,21 @@ class InvalidScanFileError(ValueError): parseable file value (ambiguous). The message always names the remedy, because the operator holding it is the only one who can act. + `reason` carries the same cause as a closed, non-PII + `ScanFileInvalidReason` a caller can branch on (whether to keep + retrying, for instance) without parsing `message` or risking a path + it may embed; see that enum for the permanent/transient split. + `message` remains the sole content of `str(exc)`, unchanged, so + every existing catcher that ignores `reason` behaves identically. + Subclasses ValueError so the shared schema validator can raise it directly for the declared evidence shape. """ + def __init__(self, message: str, *, reason: ScanFileInvalidReason) -> None: + super().__init__(message) + self.reason = reason + class UnauthorizedError(Exception): """The Authorize port denied the command.""" diff --git a/apps/api/src/cora/data/features/ingest_scan/handler.py b/apps/api/src/cora/data/features/ingest_scan/handler.py index 3853db15911..50e599c600c 100644 --- a/apps/api/src/cora/data/features/ingest_scan/handler.py +++ b/apps/api/src/cora/data/features/ingest_scan/handler.py @@ -67,7 +67,7 @@ ProducingRunNotFoundError, ) from cora.data.aggregates.distribution import DistributionSupplyNotFoundError -from cora.data.errors import InvalidScanFileError, UnauthorizedError +from cora.data.errors import InvalidScanFileError, ScanFileInvalidReason, UnauthorizedError from cora.data.features.ingest_scan.command import IngestScan from cora.data.ports.checksum_computer import ChecksumComputer from cora.data.ports.checksum_verifier import Unreachable @@ -209,7 +209,8 @@ async def handler( if resolved_locator is None: raise InvalidScanFileError( "scan file locator could not be resolved: the referenced " - "run's capture path is missing or no longer matches." + "run's capture path is missing or no longer matches.", + reason=ScanFileInvalidReason.LOCATOR_UNRESOLVED, ) # Whether resolution actually substituted a DIFFERENT string: # `described.reason` / `computed.error_detail` below come from @@ -230,19 +231,22 @@ async def handler( raise InvalidScanFileError( "scan file is not readable" + _redacted_suffix(described.reason, redact=locator_was_resolved) - + ". If the file is still transferring, retry once it has arrived." + + ". If the file is still transferring, retry once it has arrived.", + reason=ScanFileInvalidReason.UNREADABLE, ) if isinstance(described, Unrecognized): raise InvalidScanFileError( "not a recognizable scan file" - + _redacted_suffix(described.reason, redact=locator_was_resolved) + + _redacted_suffix(described.reason, redact=locator_was_resolved), + reason=ScanFileInvalidReason.UNRECOGNIZED, ) if not described.structurally_complete: raise InvalidScanFileError( "scan file is structurally incomplete: the rotation-angle " "dataset is absent, meaning post-processing has not " "completed (not yet run, failed, or zero projections). " - "Ingest the file once its writer has finished with it." + "Ingest the file once its writer has finished with it.", + reason=ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE, ) captured_at, captured_at_source = _resolve_captured_at(command, described) @@ -255,13 +259,15 @@ async def handler( if isinstance(computed, Unreachable): raise InvalidScanFileError( "could not digest scan file" - + _redacted_suffix(computed.error_detail, redact=locator_was_resolved) + + _redacted_suffix(computed.error_detail, redact=locator_was_resolved), + reason=ScanFileInvalidReason.DIGEST_UNREACHABLE, ) if (computed.byte_size, computed.mtime_ns) != (described.byte_size, described.mtime_ns): raise InvalidScanFileError( "scan file changed while being read (size or mtime moved " "between the structural read and the digest pass). It is " - "still being written or transferred; retry once it is final." + "still being written or transferred; retry once it is final.", + reason=ScanFileInvalidReason.CHANGED_WHILE_READING, ) # 3. Natural-key duplicate check: the digest, not the uri, since @@ -414,7 +420,8 @@ def _resolve_captured_at(command: IngestScan, described: Description) -> tuple[A f"captured_at was supplied but the file carries its own " f"parseable timestamp ({described.captured_at_raw}, from " f"{described.captured_at_source}). Drop the supplied value; " - f"the file's own timestamp always wins." + f"the file's own timestamp always wins.", + reason=ScanFileInvalidReason.CAPTURED_AT_AMBIGUOUS, ) return described.captured_at, described.captured_at_source if command.captured_at is not None: @@ -427,7 +434,8 @@ def _resolve_captured_at(command: IngestScan, described: Description) -> tuple[A raise InvalidScanFileError( f"the file's acquisition timestamp is {detail}. Supply captured_at " f"with the operator-asserted capture time (logbook, folder date) to " - f"ingest this file; CORA never fabricates one." + f"ingest this file; CORA never fabricates one.", + reason=ScanFileInvalidReason.CAPTURED_AT_MISSING, ) @@ -470,7 +478,8 @@ def _filename_of(locator: str) -> str: if not name: raise InvalidScanFileError( "the locator carries no filename to name the Dataset after; " - "point it at the scan file itself, not a directory." + "point it at the scan file itself, not a directory.", + reason=ScanFileInvalidReason.LOCATOR_MISSING_FILENAME, ) return name diff --git a/apps/api/tests/unit/api/test_capture_scan_ingestor.py b/apps/api/tests/unit/api/test_capture_scan_ingestor.py index 575d8bba02f..f2cb1923d83 100644 --- a/apps/api/tests/unit/api/test_capture_scan_ingestor.py +++ b/apps/api/tests/unit/api/test_capture_scan_ingestor.py @@ -46,7 +46,7 @@ ProducingRunNotFoundError, ) from cora.data.aggregates.distribution import DistributionSupplyNotFoundError -from cora.data.errors import InvalidScanFileError, UnauthorizedError +from cora.data.errors import InvalidScanFileError, ScanFileInvalidReason, UnauthorizedError from cora.infrastructure.capture_scan_ingestor_binding import ( CaptureScanIngestorBinding, CaptureScanIngestorLocation, @@ -422,7 +422,10 @@ async def test_authorization_recovering_after_a_denial_logs_the_recovery() -> No "raises", [ DatasetAlreadyIngestedError(uuid4(), "deadbeef"), - InvalidScanFileError("scan file is structurally incomplete"), + InvalidScanFileError( + "scan file is structurally incomplete", + reason=ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE, + ), ProducingRunNotFoundError(_RUN_ID), AcquisitionAssetNotFoundError(_ASSET_ID), DistributionSupplyNotFoundError(_SUPPLY_ID), @@ -463,13 +466,16 @@ async def test_tick_propagates_cancellation_instead_of_swallowing_it() -> None: @pytest.mark.unit async def test_a_failed_ingest_never_logs_the_observed_path() -> None: """`observed_path` is personal data and this log sink cannot be - erased; every failure mode's log line must carry `run_id` / - `capture_code` only, never the path or an exception message that - embeds it (`InvalidScanFileError`'s text does, via `repr()`).""" + erased; every failure mode's log line must never carry the path or + an exception message that embeds it (`InvalidScanFileError`'s text + does, via `repr()`), even though `invalid_scan_file` now also + carries `reason`, the closed `ScanFileInvalidReason` enum that by + construction can never hold a path fragment.""" lookup = _ListCandidateLookup([_candidate()]) ingest_scan = _FakeIngestScan( raises=InvalidScanFileError( - f"scan file is not readable: /local1/2BM/2026-08-{_PERSONAL_PATH_FRAGMENT}/scan_005.h5" + f"scan file is not readable: /local1/2BM/2026-08-{_PERSONAL_PATH_FRAGMENT}/scan_005.h5", + reason=ScanFileInvalidReason.UNREADABLE, ) ) ingestor = CaptureScanIngestor( @@ -484,6 +490,34 @@ async def test_a_failed_ingest_never_logs_the_observed_path() -> None: assert _PERSONAL_PATH_FRAGMENT not in str(value) +@pytest.mark.unit +async def test_invalid_scan_file_log_carries_the_reason_enum_not_the_message() -> None: + """Proves the property this change exists for: an operator reading + the warning must be able to tell WHY ingest refused without opening + the file by hand. `reason` is the enum value; the message text + stays deliberately absent (see the sibling never-logs-the-path + test).""" + lookup = _ListCandidateLookup([_candidate()]) + ingest_scan = _FakeIngestScan( + raises=InvalidScanFileError( + "scan file is structurally incomplete: the rotation-angle dataset is absent", + reason=ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE, + ) + ) + ingestor = CaptureScanIngestor( + deps=_deps(), candidate_lookup=lookup, ingest_scan=ingest_scan, bindings=_bindings() + ) + + with structlog.testing.capture_logs() as logs: + await ingestor.tick() + + entries = [e for e in logs if e["event"] == "capture_scan_ingestor.invalid_scan_file"] + assert len(entries) == 1 + assert entries[0]["reason"] == ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE.value + for value in entries[0].values(): + assert "rotation-angle" not in str(value) + + @pytest.mark.unit async def test_tick_with_no_location_for_the_candidate_root_skips_ingest() -> None: """A capture code CAN have a binding and still have no location for diff --git a/apps/api/tests/unit/data/test_errors.py b/apps/api/tests/unit/data/test_errors.py new file mode 100644 index 00000000000..e23db508ab6 --- /dev/null +++ b/apps/api/tests/unit/data/test_errors.py @@ -0,0 +1,74 @@ +"""Unit tests for `cora.data.errors`. + +`ScanFileInvalidReason` is the discriminated, non-PII reason +`InvalidScanFileError` carries so a caller can branch on cause without +parsing `str(exc)` or ever touching a path it may embed. These tests +pin the two properties that make that safe to rely on: the +permanent/transient split is real, not a single bucket everything falls +into, and every member's value stays safe to log and persist forever. +""" + +import pytest + +from cora.data.errors import ScanFileInvalidReason + +pytestmark = pytest.mark.unit + +# Mirrors the deny-list STYLE of _PII_FIELD_NAMES in +# tests/architecture/test_run_events_carry_no_pii.py, duplicated rather +# than imported because that module fitness-tests a different file's +# dataclass field NAMES; this checks arbitrary substrings of an enum +# VALUE instead, a different shape of check over the same hazard. +# Compared case-folded against the PascalCase value, so "Path" and +# "PATH" are both caught. +_PII_TOKENS = frozenset( + { + "path", + "directory", + "surname", + "lastname", + "proposal", + "esaf", + "username", + "userbadge", + "useremail", + "institution", + } +) + + +def test_scan_file_invalid_reason_transient_and_permanent_members_are_distinct() -> None: + """Mutation check: an enum where every member shared one + `is_transient` verdict would still let a badly-collapsed handler + compile and run, silently condemning transient failures as + permanent. Asserting both buckets are populated, disjoint, and that + a representative transient/permanent pair differ is the failure + this guards against; `test_ingest_scan_handler.py`'s + `test_ingest_transient_and_permanent_refusals_yield_different_reasons` + is the stronger sibling, driven through the actual handler's raise + sites rather than the enum definition alone.""" + transient = {member for member in ScanFileInvalidReason if member.is_transient} + permanent = {member for member in ScanFileInvalidReason if not member.is_transient} + + assert transient, "at least one member must be transient" + assert permanent, "at least one member must be permanent" + assert transient.isdisjoint(permanent) + assert ScanFileInvalidReason.UNREADABLE != ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE + assert ScanFileInvalidReason.UNREADABLE.is_transient + assert not ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE.is_transient + + +def test_scan_file_invalid_reason_values_carry_no_path_or_pii_token() -> None: + """Every member must stay safe to log and persist forever: per the + class docstring it must never embed, interpolate, or derive from a + filesystem path. Checks a literal path separator and a PII-style + token separately, since a future member named carelessly (for + instance around a vault path) could dodge one guard while failing + the other.""" + for member in ScanFileInvalidReason: + value = member.value + assert "/" not in value + assert "\\" not in value + lowered = value.lower() + hits = {token for token in _PII_TOKENS if token in lowered} + assert not hits, f"{member.name}'s value {value!r} contains PII-style token(s): {hits}" diff --git a/apps/api/tests/unit/data/test_ingest_scan_handler.py b/apps/api/tests/unit/data/test_ingest_scan_handler.py index cdb5b1a46f8..6a5557f9749 100644 --- a/apps/api/tests/unit/data/test_ingest_scan_handler.py +++ b/apps/api/tests/unit/data/test_ingest_scan_handler.py @@ -20,7 +20,7 @@ ) from cora.data.aggregates.dataset import DatasetAlreadyIngestedError from cora.data.aggregates.distribution import DistributionCannotRegisterOnNonStorageSupplyError -from cora.data.errors import InvalidScanFileError, UnauthorizedError +from cora.data.errors import InvalidScanFileError, ScanFileInvalidReason, UnauthorizedError from cora.data.features import ingest_scan from cora.data.features.ingest_scan import IngestScan from cora.data.features.ingest_scan.handler import DATA_EXCHANGE_PROFILE, DatasetByChecksumLookup @@ -379,6 +379,40 @@ async def test_ingest_incomplete_file_refusal_leaves_zero_events() -> None: assert await _stream_counts(store) == (0, 0, 0) +async def test_ingest_transient_and_permanent_refusals_yield_different_reasons() -> None: + """Mutation check: this is the test that fails if the unreadable and + the structurally-incomplete raise sites are ever collapsed onto the + same `ScanFileInvalidReason` member. `Unreadable` is the site + `ScanReader`'s own port docstring calls possibly transient (a + half-transferred file); a structurally incomplete file (no + rotation-angle dataset) is the site `DataExchangeScanReader` documents + as a layout verdict, not an I/O timing issue -- see + `ScanFileInvalidReason`'s class docstring for the citations behind + each classification.""" + transient_handler = _bind( + _deps(InMemoryEventStore()), described=Unreadable(reason="half-copied") + ) + permanent_handler = _bind( + _deps(InMemoryEventStore()), + described=_description(structurally_complete=False, projection_angles_deg=None), + ) + + with pytest.raises(InvalidScanFileError) as transient_exc: + await transient_handler( + _command(), principal_id=_PRINCIPAL_ID, correlation_id=_CORRELATION_ID + ) + with pytest.raises(InvalidScanFileError) as permanent_exc: + await permanent_handler( + _command(), principal_id=_PRINCIPAL_ID, correlation_id=_CORRELATION_ID + ) + + assert transient_exc.value.reason == ScanFileInvalidReason.UNREADABLE + assert permanent_exc.value.reason == ScanFileInvalidReason.STRUCTURALLY_INCOMPLETE + assert transient_exc.value.reason != permanent_exc.value.reason + assert transient_exc.value.reason.is_transient + assert not permanent_exc.value.reason.is_transient + + async def test_ingest_reader_names_an_unrecognized_captured_at_source_refuses() -> None: """`Description.captured_at_source` is a plain str so a future layout can name a timestamp no reader has produced yet (its own docstring);