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);