From be66ef034e93fb1ee3c8cd16298d1b2197b85d41 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 09:11:02 -0400 Subject: [PATCH 01/14] feat: Fold a processor identity into the workflow cache key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cache_key identified the producing processor only by its version number, never by which processor it is. Two processors claiming the same file and artifact kind at equal processor_version therefore derived an identical key and read back each other's artifacts as cache hits — a wrong answer rather than a miss. Nothing collided, but only by accident of the registry: TabixIntervalProcessor and BamIndexProcessor both sit at version 2 and stayed apart solely because their supported_formats are disjoint. Keys now carry the producing processor's identity as its own segment: {dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{version} Processor gains a processor_id class attribute, defaulted per subclass to its own class name so a processor that declares nothing still derives distinct keys — forgetting is safe rather than silently aliasing. The three shipped processors pin explicit identities instead, so the values survive a class rename; changing one invalidates every artifact keyed under it. The default is read from the subclass's own __dict__ rather than the MRO, because a subclass may emit different bytes at the same version, and inheriting the parent's identity would recreate exactly the aliasing the segment prevents. An identity is validated when the class is declared rather than at first use, so a malformed value fails on import instead of surfacing per-request inside a worker. Beyond the separator and null-byte guards normalize_local_id already applies, an identity may not be "." or ".." — caught today only by the cache backend, a different module with a different rationale — nor equal an artifact kind, which would let an over-specified purge prefix strip a live key down to something shaped exactly like a retired one. is_legacy_cache_key describes the retired four-segment scheme for the migration sweep. It requires a real artifact kind in the third segment, not merely the right segment count: it gates an irreversible delete, so a false positive is data loss while a false negative only leaves a stale object behind. That check is also what makes a mis-specified purge prefix safe, since stripping one segment too many leaves the processor identity where an artifact kind must be. --- README.md | 2 +- src/cfdb/workflows/keys.py | 137 ++++++++++++++++--- src/cfdb/workflows/processors/bam.py | 1 + src/cfdb/workflows/processors/base.py | 50 ++++++- src/cfdb/workflows/processors/passthrough.py | 1 + src/cfdb/workflows/processors/tabix.py | 1 + 6 files changed, 170 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 1452b2c..4ff7645 100644 --- a/README.md +++ b/README.md @@ -877,7 +877,7 @@ The preprocessed artifact is the default response. Clients that want the raw ups | GTF | GTF→GFF3 + sort + bgzip + tabix | bgzipped GFF3 + TBI | | bigBed | bigBedToBed + sort + bgzip + tabix | bgzipped BED + TBI | -Cache keys are content-addressed using each file's upstream `md5`, so a byte change upstream (with the sync pipeline refreshing `md5`) invalidates the cache automatically. +Cache keys have the shape `{dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{processor_version}` — for example `encode/ENCFF732YBO/index/tabix-interval/6fccbb438a046075cb438f84d0defe8d-v2`. They are content-addressed using each file's upstream `md5`, so a byte change upstream (with the sync pipeline refreshing `md5`) invalidates the cache automatically, and they carry the producing processor's identity (`Processor.processor_id`) so two processors claiming the same file and artifact kind can never read back each other's output. Without that segment a version number was the only thing separating processors — and `TabixIntervalProcessor` and `BamIndexProcessor` both sit at version 2, staying apart only because their `supported_formats` happen to be disjoint. A processor that declares no `processor_id` inherits its own class name; declare one explicitly when the identity should survive a class rename, since changing the string invalidates every artifact keyed under it. The declaration must sit in the processor's own class body — a value supplied by a base class or mixin is discarded in favour of the class name, so factoring a pinned identity into a mixin would silently cold-cache everything keyed under it. An identity may not be blank, contain a path separator, be `.` or `..`, or equal an artifact kind (`data`, `index`); each is rejected when the class is declared, and the last of those keeps a mis-specified purge prefix from reducing a live key to something shaped like a retired one. **Bounded concurrency, durable queuing, and admission control.** Dispatch is bounded on three cooperating layers so an unauthenticated burst on `/data` and `/index` can't oversubscribe the worker fleet or queue unbounded work: diff --git a/src/cfdb/workflows/keys.py b/src/cfdb/workflows/keys.py index 455d7aa..dd85f2e 100644 --- a/src/cfdb/workflows/keys.py +++ b/src/cfdb/workflows/keys.py @@ -8,9 +8,11 @@ active (pending/running) state at any time. - `cache_key` identifies a single cacheable artifact produced by a workflow. - It is scoped per-artifact-kind so that the data artifact and the index - artifact land in separate cache entries sharing the same source-file - lineage. + It is scoped per-artifact-kind *and* per-producing-processor so that the + data artifact and the index artifact land in separate cache entries + sharing the same source-file lineage, and so that two processors claiming + the same (file, artifact_kind) pair can never read back each other's + output. Keys are content-addressed via `md5` so that an upstream byte change (with its md5 refreshed in the metadata sync) automatically invalidates cached @@ -26,6 +28,24 @@ _MD5_HEX_RE = re.compile(r"^[a-f0-9]{32}$") +#: Leaf shape shared by the current and the retired cache-key schemes: +#: the content address followed by the producing processor's version. +_CACHE_LEAF_RE = re.compile(r"^[a-f0-9]{32}-v\d+$") + +#: Segment count of the retired (pre-#109) cache key +#: ``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}``. The +#: current scheme carries a processor-identity segment and so is one +#: longer; see :func:`is_legacy_cache_key`. +_LEGACY_KEY_SEGMENTS = 4 + +#: Position of the artifact-kind segment in the retired key. +_LEGACY_KIND_INDEX = 2 + +#: The legal artifact-kind segment values, as strings. Both key schemes +#: place one here, so it is the segment that tells a cache key apart from +#: an unrelated object sharing the bucket. +_ARTIFACT_KIND_VALUES = frozenset(kind.value for kind in ArtifactKind) + def normalize_dcc(dcc: str) -> str: """Canonical DCC form used by both ``workflow_key`` and ``cache_key``. @@ -70,6 +90,86 @@ def normalize_local_id(local_id: str) -> str: return cleaned +def normalize_processor_id(processor_id: str) -> str: + """Canonical processor-identity form embedded in ``cache_key``. + + Strips whitespace and preserves case. Rejects an empty (or + whitespace-only) value and any path-separator or null-byte character, + for the same reason ``normalize_local_id`` does: the value becomes a + path segment in the cache key, and a stray ``/`` would silently + restructure the key rather than fail. + + Case is preserved because the default identity is a processor's class + name (see ``Processor.__init_subclass__``), and folding case would + merge ``BedProcessor`` with a hypothetical ``BEDProcessor``. + + Two further values are rejected, both because of what they would do to + :func:`is_legacy_cache_key` rather than to the key itself: + + - ``"."`` and ``".."`` traverse a path segment without containing a + separator. ``cache.py``'s ``_validate_cache_key`` already refuses + them at ``put`` / ``head`` time, but that surfaces as a failure deep + inside a workflow; rejecting here fails at derivation instead. + - A value equal to an :class:`ArtifactKind` would let an + over-specified purge prefix strip a live key down to something + shaped exactly like a retired one — the processor id would land in + the artifact-kind slot and satisfy that segment's check. See + :func:`is_legacy_cache_key`. + """ + cleaned = processor_id.strip() if processor_id else "" + if not cleaned: + raise ValueError("processor_id is required for cache key derivation") + if "/" in cleaned or "\\" in cleaned or "\x00" in cleaned: + raise ValueError(f"processor_id contains forbidden chars: {processor_id!r}") + if cleaned in (".", ".."): + raise ValueError(f"processor_id must not be a path traversal: {processor_id!r}") + if cleaned in _ARTIFACT_KIND_VALUES: + raise ValueError( + f"processor_id must not collide with an artifact kind: " + f"{processor_id!r} (it would make an over-stripped cache key " + f"indistinguishable from a retired one)" + ) + return cleaned + + +def is_legacy_cache_key(key: str) -> bool: + """Return True when ``key`` was minted under the retired cache scheme. + + The retired scheme (everything written before issue #109) was + ``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}`` — four + segments, with no processor identity. The current scheme inserts that + identity ahead of the leaf, so a legacy key is a four-segment key + whose third segment is an artifact kind and whose leaf is a content + address plus a version. + + Nothing reads legacy keys any more: every lookup goes through + :func:`cache_key`, which now derives the five-segment form. This + predicate is the single description of the retired shape, consumed by + the ``cfdb purge-legacy-cache`` sweep in + :mod:`cfdb.workflows.purge`. + + Because that sweep deletes what this returns True for, the checks are + deliberately narrow — a false positive is unrecoverable data loss, + while a false negative only leaves a stale object behind. Requiring + the artifact-kind segment is what makes the predicate safe under a + mis-specified purge prefix: ``purge_s3`` strips the configured prefix + before testing, so a prefix carrying one segment too many would + otherwise reduce a *live* five-segment key to a four-segment one and + delete it. With this check the processor identity lands in the + artifact-kind slot and fails; stripping two or more segments leaves + too few to match at all. :func:`normalize_processor_id` forbids an + identity equal to an artifact kind, closing the remaining overlap. + """ + segments = key.split("/") + if len(segments) != _LEGACY_KEY_SEGMENTS: + return False + if not all(segments[:-1]): + return False + if segments[_LEGACY_KIND_INDEX] not in _ARTIFACT_KIND_VALUES: + return False + return bool(_CACHE_LEAF_RE.fullmatch(segments[-1])) + + def extract_identity(file_meta: dict[str, Any]) -> tuple[str, str, str]: """Pull canonical (dcc, local_id, md5) from a file metadata dict. @@ -159,6 +259,7 @@ def cache_key( local_id: str, artifact_kind: ArtifactKind, md5: str, + processor_id: str, processor_version: int, ) -> str: """Build the cache key for a single workflow output artifact. @@ -169,6 +270,10 @@ def cache_key( artifact_kind: Which artifact kind (data or index) this key addresses. md5: MD5 hex digest of the upstream file bytes. + processor_id: Stable identity of the processor that produced the + artifact (``Processor.processor_id``). Two processors claiming + the same ``(file, artifact_kind)`` pair derive different keys + because of this segment, whatever their versions are. processor_version: Monotonically-increasing version of the processor implementation that produced the artifact. Bumping this value invalidates cached outputs for the corresponding processor @@ -176,7 +281,7 @@ def cache_key( Returns: A stable string key of the form - ``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}``. + ``{dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{processor_version}``. Note: ``cache_key`` deliberately does NOT include ``pipeline_version``. @@ -186,20 +291,15 @@ def cache_key( To force fresh cache entries for a single processor's outputs, bump that processor's ``processor_version`` instead. - Warning: - The key identifies the processor only by ``processor_version``, not - by which processor it is. Two processors that claim the same - ``(file, artifact_kind)`` pair at equal ``processor_version`` derive - the *same* key and would read back each other's artifacts as cache - hits -- a wrong answer rather than a miss. This holds today only - because each pair is claimed by at most one processor, which is a - property of the current registry and not of this function. Fold a - processor identity (class name, or a registry-assigned id) into the - key before landing a second processor for any pair. The paired - interval formats make this concrete: ``.bedpe`` and ``bigInteract`` - files carry ``index`` artifacts built by ``TabixIntervalProcessor`` - before they were re-typed, so a future paired-interval processor is - exactly the case that would collide. + Note: + ``processor_id`` is what keeps two processors apart, and it is + carried here rather than left to the registry because the version + alone cannot do the job: ``TabixIntervalProcessor`` and + ``BamIndexProcessor`` both sit at version 2 and stayed apart only + because their ``supported_formats`` happen to be disjoint (issue + #109). Keys minted before this segment existed are recognised by + :func:`is_legacy_cache_key` and swept by ``cfdb + purge-legacy-cache``; nothing derives or reads them any more. """ if processor_version < 0: raise ValueError("processor_version must be non-negative") @@ -207,5 +307,6 @@ def cache_key( f"{normalize_dcc(dcc)}/" f"{normalize_local_id(local_id)}/" f"{artifact_kind.value}/" + f"{normalize_processor_id(processor_id)}/" f"{normalize_md5(md5)}-v{processor_version}" ) diff --git a/src/cfdb/workflows/processors/bam.py b/src/cfdb/workflows/processors/bam.py index 16483ba..39dae69 100644 --- a/src/cfdb/workflows/processors/bam.py +++ b/src/cfdb/workflows/processors/bam.py @@ -133,6 +133,7 @@ class BamIndexProcessor(Processor): ``ArtifactKind.INDEX``. """ + processor_id = "bam-index" processor_version = 2 supported_formats = frozenset({"BAM", "SAM"}) #: Class-level default. Real per-file advertisement comes from diff --git a/src/cfdb/workflows/processors/base.py b/src/cfdb/workflows/processors/base.py index b7197c0..35902f0 100644 --- a/src/cfdb/workflows/processors/base.py +++ b/src/cfdb/workflows/processors/base.py @@ -43,6 +43,22 @@ class Processor(ABC): across the Wool boundary. """ + #: Stable identity of this processor, baked into its cache keys so two + #: processors claiming the same ``(file, artifact_kind)`` pair can never + #: read back each other's artifacts (issue #109). Subclasses that do + #: NOT declare one inherit their own class name via + #: ``__init_subclass__`` — distinct by default, so forgetting is safe. + #: Declare an explicit value when the identity should survive a class + #: rename: changing this string invalidates every artifact the + #: processor has cached. + #: + #: The declaration MUST sit in the processor's own class body. The + #: default is applied from ``cls.__dict__``, not the MRO, so a value + #: supplied by a mixin or a base class is discarded in favour of the + #: class name — factoring a pinned identity out into a mixin would + #: silently cold-cache every artifact keyed under it. + processor_id: str = "" + #: Class-level version. Bump when the processor's output-producing #: logic changes in any way that affects the artifact bytes. This is #: baked into cache keys so bumps naturally trigger reprocessing. @@ -55,6 +71,31 @@ class Processor(ABC): #: them. For most pipelines this is ``[DATA, INDEX]``. artifact_kinds: tuple[ArtifactKind, ...] = () + def __init_subclass__(cls, **kwargs: Any) -> None: + """Default ``processor_id`` to the subclass's own class name. + + Only a value declared in the subclass's **own body** counts — an + inherited one is replaced. A subclass of a shipped processor + produces potentially different bytes under the same + ``processor_version``, so inheriting the parent's identity would + recreate exactly the aliasing the identity exists to prevent. A + declared-but-falsy value (``processor_id = ""``) is treated as no + declaration at all and takes the class-name default, because an + empty identity is the same failure as a missing one. + + A declared value is validated here rather than at first use, so a + malformed identity (``" "``, ``".."``, one colliding with an + artifact kind) raises when the module is imported instead of + surfacing per-request inside a worker, long after the class that + caused it was written. + """ + super().__init_subclass__(**kwargs) + declared = cls.__dict__.get("processor_id") + if not declared: + cls.processor_id = cls.__name__ + else: + cls.processor_id = key_utils.normalize_processor_id(declared) + def artifact_kinds_produced( self, file_meta: dict[str, Any] | None = None ) -> tuple[ArtifactKind, ...]: @@ -80,9 +121,11 @@ def cache_key_for( the cache with this key, the processor ``put``s under it, and the :class:`~cfdb.workflows.events.StageComplete` event carries it — so all three agree by construction rather than by three independent - re-derivations that must be kept in sync. ``processor_version`` is - baked in, so bumping it invalidates this processor's cached - artifacts without disturbing other processors'. + re-derivations that must be kept in sync. Both ``processor_id`` and + ``processor_version`` are baked in: the identity keeps this + processor's artifacts disjoint from every other processor's even at + an equal version, and bumping the version invalidates this + processor's own cached artifacts without disturbing anyone else's. Raises ``ValueError`` (via :func:`extract_identity`) when ``file_meta`` is missing dcc / local_id / md5. @@ -93,6 +136,7 @@ def cache_key_for( local_id=local_id, artifact_kind=artifact_kind, md5=md5, + processor_id=self.processor_id, processor_version=self.processor_version, ) diff --git a/src/cfdb/workflows/processors/passthrough.py b/src/cfdb/workflows/processors/passthrough.py index 5e7cdf9..cc0c77f 100644 --- a/src/cfdb/workflows/processors/passthrough.py +++ b/src/cfdb/workflows/processors/passthrough.py @@ -20,6 +20,7 @@ class PassthroughProcessor(Processor): returns False and ``run`` is never invoked. """ + processor_id = "passthrough" processor_version = 0 supported_formats = frozenset({"CSV", "TSV", "bigWig"}) artifact_kinds = () diff --git a/src/cfdb/workflows/processors/tabix.py b/src/cfdb/workflows/processors/tabix.py index 1c5af12..1b86c61 100644 --- a/src/cfdb/workflows/processors/tabix.py +++ b/src/cfdb/workflows/processors/tabix.py @@ -167,6 +167,7 @@ class TabixIntervalProcessor(Processor): # ever be committed, so re-key all tabix artifacts — a poisoned v1 # ``data`` entry (committed before the guard existed) becomes a cache # miss, re-enters _stage_prepare, and is rejected instead of served. + processor_id = "tabix-interval" processor_version = 2 supported_formats = frozenset(_TABIX_PRESET.keys()) artifact_kinds = (ArtifactKind.DATA, ArtifactKind.INDEX) From 0238e5c3069d27c01fcdfbf972e60aca04e5d28d Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 09:11:13 -0400 Subject: [PATCH 02/14] feat: Reject two processors that share an identity at registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The class-name default keeps processors that declare nothing apart, but it cannot catch the case that actually matters: two distinct classes pinning the same explicit processor_id, as a new processor copy-pasting a shipped identity would. Cache keys are scoped by that identity, so the two would read back each other's artifacts as cache hits. Rejecting at wiring time makes the property an enforced invariant rather than a convention every new processor has to remember, and fails in CI instead of at API startup. The guard keys on the identity alone, so two processors claiming one format remain legal and order-resolved — the documented first-registration-wins contract is unchanged. --- src/cfdb/workflows/processors/registry.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/cfdb/workflows/processors/registry.py b/src/cfdb/workflows/processors/registry.py index bb633d1..0255a7b 100644 --- a/src/cfdb/workflows/processors/registry.py +++ b/src/cfdb/workflows/processors/registry.py @@ -30,7 +30,24 @@ def register(self, processor: Processor) -> None: returns the first processor whose ``supported_formats`` covers the file's format name, so callers should register more specific processors before more general ones. + + Raises: + ValueError: Another registered processor already claims this + one's ``processor_id``. Cache keys are scoped by that + identity (issue #109), so two processors sharing it would + read back each other's artifacts as cache hits — a wrong + answer rather than a miss. Rejecting at wiring time turns + the property into an enforced invariant instead of a + convention each new processor has to remember. """ + for registered in self._processors: + if registered.processor_id == processor.processor_id: + raise ValueError( + f"processor_id {processor.processor_id!r} is already " + f"registered by {type(registered).__name__}; cache keys " + f"are scoped by this identity, so " + f"{type(processor).__name__} would alias its artifacts" + ) self._processors.append(processor) def lookup_for(self, file_meta: dict[str, Any]) -> Processor | None: From 52eb1b35b26a3c3fdc655dea8ed0aa7fa73bf021 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 09:11:28 -0400 Subject: [PATCH 03/14] feat: Add a sweep for cache entries under the retired key scheme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folding a processor identity into the cache key invalidates every existing artifact by construction — the API derives the five-segment shape and never probes the old one — so the entries are unreachable storage cost until swept. The sweep also clears the orphaned .bedpe and bigInteract index artifacts stranded when those formats were re-typed; they are retired-scheme keys too, so they need no separate pass. Both backends are covered because a deployment runs one or the other, and cfdb purge-legacy-cache exposes them through the installed console script so the sweep can run from a deployed image. It is a dry run unless --apply is passed, and it refuses to act when both an S3 bucket and a local root resolve rather than guessing which cache was meant — WORKFLOW_S3_BUCKET in the environment is enough to reach that state alongside an explicit --local-root, and purging the wrong store cannot be undone. Two failure modes are surfaced rather than absorbed. DeleteObjects reports per-key failures in the response body instead of raising, so a sweep that deleted nothing for want of an s3:DeleteObject grant would otherwise report success; and a response confirming fewer keys than were requested means something was neither deleted nor complained about. Both raise, and the sweep is idempotent, so re-running after fixing the cause picks up whatever is left. The local sweep prunes only the directories its own deletions emptied. The root is operator-supplied, so an unrelated empty directory under it is not the sweep's to reclaim. --- ENCODE-SUPPLEMENT.md | 2 +- README.md | 23 ++++ src/cfdb/cli.py | 112 ++++++++++++++++++++ src/cfdb/workflows/purge.py | 206 ++++++++++++++++++++++++++++++++++++ 4 files changed, 342 insertions(+), 1 deletion(-) create mode 100644 src/cfdb/workflows/purge.py diff --git a/ENCODE-SUPPLEMENT.md b/ENCODE-SUPPLEMENT.md index e309fe7..bef7132 100644 --- a/ENCODE-SUPPLEMENT.md +++ b/ENCODE-SUPPLEMENT.md @@ -61,7 +61,7 @@ ENCODE uses human-readable strings for file formats, assay types, output types, This applies to `.bedpe` already in the ENCODE experiment corpus, not only to annotation files — but it reaches no further than ENCODE. `FILE_FORMAT_TO_EDAM` and `get_file_format` have exactly one consumer, the ENCODE transform; 4DN and HuBMAP take `file_format` from their upstream C2M2 datapackage or portal API and never consult the table. A 4DN `.bedpe` whose upstream declares BED therefore still carries `file_format.name == "BED"`, is still claimed by `TabixIntervalProcessor`, and still gets an index built from its first mate. Closing that means routing incoming formats from every DCC through the same table, or refusing a `.bedpe` filename at the processor regardless of declared format; both are follow-up work. -**Cache artifacts left behind.** Files of these two formats that were indexed before the re-typing still have their incorrect `.tbi` artifacts in the workflow cache. Nothing reads them any more — `lookup_for` returns `None` and the router bails before probing the cache — and nothing purges them either, so they are unreachable storage cost until someone sweeps them. They also make a latent cache-key hazard concrete: `cache_key` identifies a processor only by its `processor_version`, not by which processor it is, so a future paired-interval processor sharing a version number with `TabixIntervalProcessor` would derive the same key and read those stale artifacts back as cache hits. A processor identity has to be folded into the key before that processor lands; see the warning on `cfdb.workflows.keys.cache_key`. +**Cache artifacts left behind.** Files of these two formats that were indexed before the re-typing still have their incorrect `.tbi` artifacts in the workflow cache. Nothing reads them — `lookup_for` returns `None` and the router bails before probing the cache. The hazard they posed is closed: `cache_key` used to identify a processor only by its `processor_version`, so a future paired-interval processor sharing a version number with `TabixIntervalProcessor` would have derived the same key and read those stale artifacts back as cache hits. Issue #109 folded the producing processor's identity into the key, so that collision is now impossible rather than merely unlikely, and the stranded artifacts — which are keyed under the retired scheme — are swept by `cfdb purge-legacy-cache`. ### Output Type -> EDAM Data diff --git a/README.md b/README.md index 4ff7645..3a9271e 100644 --- a/README.md +++ b/README.md @@ -879,6 +879,8 @@ The preprocessed artifact is the default response. Clients that want the raw ups Cache keys have the shape `{dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{processor_version}` — for example `encode/ENCFF732YBO/index/tabix-interval/6fccbb438a046075cb438f84d0defe8d-v2`. They are content-addressed using each file's upstream `md5`, so a byte change upstream (with the sync pipeline refreshing `md5`) invalidates the cache automatically, and they carry the producing processor's identity (`Processor.processor_id`) so two processors claiming the same file and artifact kind can never read back each other's output. Without that segment a version number was the only thing separating processors — and `TabixIntervalProcessor` and `BamIndexProcessor` both sit at version 2, staying apart only because their `supported_formats` happen to be disjoint. A processor that declares no `processor_id` inherits its own class name; declare one explicitly when the identity should survive a class rename, since changing the string invalidates every artifact keyed under it. The declaration must sit in the processor's own class body — a value supplied by a base class or mixin is discarded in favour of the class name, so factoring a pinned identity into a mixin would silently cold-cache everything keyed under it. An identity may not be blank, contain a path separator, be `.` or `..`, or equal an artifact kind (`data`, `index`); each is rejected when the class is declared, and the last of those keeps a mis-specified purge prefix from reducing a live key to something shaped like a retired one. +**Purging the retired key scheme.** Keys minted before the processor-identity segment existed are unreachable by construction — the API derives the current shape and never probes the old one. Sweep them with `cfdb purge-legacy-cache` (dry run by default; pass `--apply` to delete). The same sweep clears the orphaned `.bedpe` / `bigInteract` index artifacts stranded when those formats were re-typed. Note that the deploy shipping the key change starts against a **fully cold cache**: every `/data` and `/index` request for a processable file dispatches a fresh workflow until the fleet catches up. + **Bounded concurrency, durable queuing, and admission control.** Dispatch is bounded on three cooperating layers so an unauthenticated burst on `/data` and `/index` can't oversubscribe the worker fleet or queue unbounded work: - **Per-worker backpressure** — each worker accepts at most `CFDB_WORKER_MAX_CONCURRENT_TASKS` tasks at once (default `1`), serializing the subprocess pipelines on a 1-vCPU worker. A worker at capacity rejects the dispatch and the API's priority load balancer rotates to the next worker. @@ -1080,3 +1082,24 @@ cfdb sync 4dn hubmap - `--api-url` - cfdb API base URL (default: `http://localhost:8000`, env: `CFDB_API_URL`) - `--api-key` - API key for sync endpoint (env: `SYNC_API_KEY`) - `--debug` / `-d` - Enable debugpy debugging + +```bash +# Report what the retired cache-key scheme is still holding (dry run) +cfdb purge-legacy-cache + +# Delete it — local cache root, then an S3-backed environment +cfdb purge-legacy-cache --local-root ./data/cache --apply +cfdb purge-legacy-cache --s3-bucket cfdb-cache --s3-prefix dev --apply +``` + +**Options:** +- `--s3-bucket` - bucket holding the workflow cache (env: `WORKFLOW_S3_BUCKET`) +- `--s3-prefix` - key prefix the S3 cache backend writes under (env: `WORKFLOW_S3_PREFIX`) +- `--endpoint-url` - boto3 endpoint override for LocalStack-backed dev (env: `AWS_ENDPOINT_URL`) +- `--region` - AWS region for the boto3 client (env: `AWS_REGION`) +- `--local-root` - local cache root (default: `$SYNC_DATA_DIR/cache`) +- `--apply` - actually delete; without it the sweep only reports + +Exactly one store is purged per run. When both an S3 bucket and a local root resolve — `WORKFLOW_S3_BUCKET` set in the environment alongside an explicit `--local-root`, say — the command refuses rather than guessing which cache you meant. + +`--s3-prefix` must be exactly the prefix the cache backend writes under (`WORKFLOW_S3_PREFIX`). A prefix carrying extra segments is stripped from every key before the retired-shape test, so an over-specified one would otherwise reduce live five-segment keys to four-segment ones; the sweep rejects those because the processor identity lands where an artifact kind must be, but the safest habit is still to pass the same value the API runs with. A prefix that is too short simply matches nothing. diff --git a/src/cfdb/cli.py b/src/cfdb/cli.py index 9de9f73..28975a5 100644 --- a/src/cfdb/cli.py +++ b/src/cfdb/cli.py @@ -1,4 +1,6 @@ import logging +import os +from pathlib import Path import click import requests @@ -110,5 +112,115 @@ def sync(dcc_names: tuple[str, ...], api_url: str, api_key: str): raise SystemExit(1) +@cli.command("purge-legacy-cache") +@click.option( + "--s3-bucket", + default=None, + envvar="WORKFLOW_S3_BUCKET", + help="Bucket holding the workflow cache (S3 profile).", +) +@click.option( + "--s3-prefix", + default="", + envvar="WORKFLOW_S3_PREFIX", + help="Key prefix the S3 cache backend writes under.", +) +@click.option( + "--endpoint-url", + default=None, + envvar="AWS_ENDPOINT_URL", + help="boto3 endpoint override (LocalStack-backed dev).", +) +@click.option( + "--region", + default=None, + envvar="AWS_REGION", + help="AWS region for the boto3 client.", +) +@click.option( + "--local-root", + default=None, + help="Local cache root. Defaults to $SYNC_DATA_DIR/cache.", + type=click.Path(file_okay=False, path_type=Path), +) +@click.option( + "--apply", + default=False, + help="Delete the matched entries. Without it the sweep is a dry run.", + is_flag=True, +) +def purge_legacy_cache( + s3_bucket: str | None, + s3_prefix: str, + endpoint_url: str | None, + region: str | None, + local_root: Path | None, + apply: bool, +): + """ + Sweep workflow cache entries minted under the retired key scheme. + + Issue #109 folded a processor identity into the cache key, so every + entry written before it is unreachable: the API derives the new key + shape and never probes the old one. This command deletes those + entries, including the orphaned .bedpe / bigInteract index artifacts + left behind when PR #108 re-typed those formats. + + Runs as a DRY RUN unless --apply is passed. The target is the S3 + bucket when one is configured, otherwise the local cache root. + + Examples: + + cfdb purge-legacy-cache + + cfdb purge-legacy-cache --local-root ./data/cache --apply + + cfdb purge-legacy-cache --s3-bucket cfdb-cache --s3-prefix dev --apply + """ + from cfdb.workflows.purge import build_s3_client, purge_local, purge_s3 + + if local_root is None and not s3_bucket: + sync_data_dir = os.getenv("SYNC_DATA_DIR") + if sync_data_dir: + local_root = Path(sync_data_dir) / "cache" + + if s3_bucket and local_root is not None: + # Both stores resolved — refuse rather than guess. WORKFLOW_S3_BUCKET + # in the environment is enough to trigger this alongside an explicit + # --local-root, and purging the wrong store is not recoverable. + raise click.UsageError( + "Both an S3 bucket and a local cache root resolved; pass only " + "one (unset WORKFLOW_S3_BUCKET to target --local-root)" + ) + if not s3_bucket and local_root is None: + raise click.UsageError( + "No cache to purge: pass --s3-bucket or --local-root, or set " + "WORKFLOW_S3_BUCKET / SYNC_DATA_DIR" + ) + + if s3_bucket: + target = f"s3://{s3_bucket}/{s3_prefix.strip('/')}".rstrip("/") + report = purge_s3( + build_s3_client(endpoint_url=endpoint_url, region_name=region), + s3_bucket, + prefix=s3_prefix, + apply=apply, + ) + else: + target = str(local_root) + report = purge_local(local_root, apply=apply) + + click.echo(f"Target: {target}") + click.echo(f"Scanned: {report.scanned}") + click.echo( + f"Legacy entries: {report.matched} " + f"({report.bytes_matched:,} bytes, {report.bytes_matched / 1024**3:.2f} GiB)" + ) + if apply: + click.echo(f"Deleted: {report.deleted}") + else: + click.echo("Dry run — nothing deleted. Re-run with --apply.") + + if __name__ == "__main__": cli() diff --git a/src/cfdb/workflows/purge.py b/src/cfdb/workflows/purge.py new file mode 100644 index 0000000..16bcf84 --- /dev/null +++ b/src/cfdb/workflows/purge.py @@ -0,0 +1,206 @@ +"""Sweep cache entries minted under the retired cache-key scheme. + +Issue #109 folded a processor identity into ``workflows.keys.cache_key``. +Every key derived under the old shape +(``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}``) is +therefore unreachable by construction: the router derives the new +five-segment form and probes that, so the old entries are never read +again and never overwritten. They are pure storage cost until swept. + +The sweep also clears the orphaned paired-interval artifacts left by PR +#108 — the incorrect ``.tbi`` files built for ``.bedpe`` / ``bigInteract`` +before those formats were re-typed. Those are old-scheme keys too, so +they need no separate pass. + +Both cache backends are covered because a deployment runs one or the +other: ``S3Cache`` in the ECS profile, ``LocalFsCache`` everywhere else. +The single description of the retired shape lives in +:func:`cfdb.workflows.keys.is_legacy_cache_key`; nothing here re-derives +it. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from cfdb.workflows.cache import _build_s3_client +from cfdb.workflows.keys import is_legacy_cache_key + +#: S3 caps a single ``DeleteObjects`` request at 1000 keys. +_S3_DELETE_BATCH = 1000 + + +def build_s3_client( + *, endpoint_url: str | None = None, region_name: str | None = None +) -> Any: + """Build the boto3 ``s3`` client :func:`purge_s3` operates through. + + Shares :mod:`cfdb.workflows.cache`'s client factory so the sweep + resolves ``endpoint_url`` / ``region_name`` exactly the way the cache + backend it is sweeping does — a LocalStack-backed dev environment + would otherwise be purged against real AWS. Leave both ``None`` to + let boto3's default session resolver pick them up. + """ + return _build_s3_client(endpoint_url=endpoint_url, region_name=region_name) + + +@dataclass +class PurgeReport: + """Accounting for one purge run. + + Attributes: + scanned: Cache entries examined. + matched: Entries recognised as legacy-scheme keys. + deleted: Entries actually removed — zero on a dry run, equal to + ``matched`` on a successful applied run. + bytes_matched: Total size of the matched entries. Reported on a + dry run too, so an operator can see what the sweep would + reclaim before committing to it. + """ + + scanned: int = 0 + matched: int = 0 + deleted: int = 0 + bytes_matched: int = 0 + + +def purge_s3( + client: Any, + bucket: str, + *, + prefix: str = "", + apply: bool = False, +) -> PurgeReport: + """Delete legacy-scheme objects from an ``S3Cache`` bucket. + + Args: + client: A boto3 ``s3`` client. + bucket: Bucket holding the workflow cache. + prefix: The cache's ``WORKFLOW_S3_PREFIX``. Stripped from each + object key before the legacy-shape test, since the prefix is + the backend's own namespacing and not part of the cache key. + apply: When False (the default) nothing is deleted and the report + describes what would be. + + Returns: + A :class:`PurgeReport` for the run. + """ + normalized = prefix.strip("/") + list_prefix = f"{normalized}/" if normalized else "" + report = PurgeReport() + batch: list[str] = [] + + paginator = client.get_paginator("list_objects_v2") + for page in paginator.paginate(Bucket=bucket, Prefix=list_prefix): + for obj in page.get("Contents", ()): + report.scanned += 1 + key = obj["Key"] + if not is_legacy_cache_key(key[len(list_prefix) :]): + continue + report.matched += 1 + report.bytes_matched += obj.get("Size", 0) + batch.append(key) + if apply and len(batch) >= _S3_DELETE_BATCH: + report.deleted += _delete_s3_batch(client, bucket, batch) + batch = [] + + if apply and batch: + report.deleted += _delete_s3_batch(client, bucket, batch) + + if apply and report.deleted != report.matched: + # ``DeleteObjects`` with ``Quiet: False`` echoes every key it + # removed, and a key that was already gone still comes back as + # deleted — so a shortfall here means S3 neither deleted nor + # complained about something, and the sweep is not the clean one + # the report would otherwise claim. Re-running is safe. + raise RuntimeError( + f"purge swept {report.matched} legacy keys but S3 confirmed only " + f"{report.deleted}; the cache was not fully purged" + ) + return report + + +def _delete_s3_batch(client: Any, bucket: str, keys: list[str]) -> int: + """Delete one batch of object keys; return how many S3 confirmed. + + ``DeleteObjects`` reports per-key failures in the response body rather + than raising, so a partial failure would otherwise pass silently as a + completed sweep. Raise instead — the sweep is idempotent, so re-running + it after fixing the cause (usually a missing ``s3:DeleteObject`` grant) + picks up whatever is left. + """ + response = client.delete_objects( + Bucket=bucket, + Delete={"Objects": [{"Key": key} for key in keys], "Quiet": False}, + ) + errors = response.get("Errors", ()) + if errors: + raise RuntimeError( + f"{len(errors)} of {len(keys)} deletions failed; first was " + f"{errors[0].get('Key')!r}: {errors[0].get('Message')}" + ) + return len(response.get("Deleted", ())) + + +def purge_local(root: Path, *, apply: bool = False) -> PurgeReport: + """Delete legacy-scheme entries from a ``LocalFsCache`` root. + + Args: + root: The cache root (``$SYNC_DATA_DIR/cache``). A root that does + not exist yields an empty report rather than an error — a + deployment that never wrote a local cache has nothing to + purge. + apply: When False (the default) nothing is deleted and the report + describes what would be. + + Returns: + A :class:`PurgeReport` for the run. Directories the deletions + emptied are pruned so the tree does not retain the shape of the + retired scheme. + """ + report = PurgeReport() + if not root.is_dir(): + return report + + emptied: list[Path] = [] + for path in sorted(root.rglob("*")): + if not path.is_file(): + continue + report.scanned += 1 + if not is_legacy_cache_key(path.relative_to(root).as_posix()): + continue + report.matched += 1 + report.bytes_matched += path.stat().st_size + if apply: + path.unlink() + report.deleted += 1 + emptied.append(path.parent) + + for directory in emptied: + _prune_empty_ancestors(directory, root) + return report + + +def _prune_empty_ancestors(directory: Path, root: Path) -> None: + """Remove ``directory`` and its now-empty parents, stopping at ``root``. + + Scoped to the ancestors of a deleted entry rather than walking the + whole tree: a sweep must not remove directories it never touched. + ``$SYNC_DATA_DIR/cache`` is operator-supplied, so an unrelated empty + directory under it is not the sweep's to reclaim. Stops at the first + ancestor that still holds something, and never removes ``root``. + """ + current = directory + while current != root and root in current.parents: + try: + if any(current.iterdir()): + return + current.rmdir() + except OSError: + # Raced with another writer, or never existed. Either way the + # directory is not ours to reclaim; the artifacts are gone, + # which is what the sweep promised. + return + current = current.parent From 463519d7e81e1f04e62f97bc8dd625201d050d27 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 09:11:45 -0400 Subject: [PATCH 04/14] test: Cover the processor identity and the retired-key predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins the property the issue asks to enforce: two processors claiming the same file and artifact kind at equal processor_version derive different keys. Covered as an example at both the keys and the Processor layer, as a Hypothesis property over the whole input domain, and against the shipped TabixIntervalProcessor and BamIndexProcessor pair the issue names as staying apart only by accident. is_legacy_cache_key gets the heaviest coverage, because it decides what an irreversible sweep deletes. Two properties bracket it: no key cache_key can mint is ever claimed, so a live artifact cannot be swept; and every key the retired scheme could mint is claimed, so the migration leaves no orphan. Around them sit the rejection cases — wrong segment counts, blank segments, foreign objects sharing the bucket, a workflow_key, and a current key stripped of its leading segment as an over-specified purge prefix produces. The shipped processor identities are asserted as literals rather than derived from the classes. They are wire constants: every cached artifact is keyed under one, the README documents "tabix-interval" inside an example key, and changing a string silently invalidates that processor's whole cached corpus. Deriving them would assert nothing. The duplicate-registration test previously registered one class twice, which the class-name default makes nearly unreachable. It now uses two distinct classes pinning one identity, and asserts the message names the identity and the incumbent, since that message is the operator's only diagnostic. The ordering test needed a second BAM-claiming class for the same reason, so it now also documents that the guard is scoped by identity rather than by format. --- tests/test_workflows/test_keys.py | 641 +++++++++++++++++- tests/test_workflows/test_processors_bam.py | 18 + tests/test_workflows/test_processors_base.py | 305 ++++++++- .../test_processors_passthrough.py | 16 + .../test_processors_registry.py | 231 ++++++- tests/test_workflows/test_processors_tabix.py | 53 ++ 6 files changed, 1244 insertions(+), 20 deletions(-) diff --git a/tests/test_workflows/test_keys.py b/tests/test_workflows/test_keys.py index 6675a3d..fe7a2f4 100644 --- a/tests/test_workflows/test_keys.py +++ b/tests/test_workflows/test_keys.py @@ -3,14 +3,16 @@ from __future__ import annotations import pytest -from hypothesis import given, settings +from hypothesis import assume, given, settings from hypothesis import strategies as st from cfdb.workflows.keys import ( cache_key, + is_legacy_cache_key, normalize_dcc, normalize_local_id, normalize_md5, + normalize_processor_id, workflow_key, ) from cfdb.workflows.models import ArtifactKind @@ -19,6 +21,35 @@ #: Mixed-case variant used to exercise normalization round-trips. _FIXTURE_MD5_UPPER = FIXTURE_MD5.upper() +#: Shared strategies for the cache-key property tests. The alphabets +#: match what ``normalize_dcc`` / ``normalize_local_id`` accept: letters +#: and digits only, so no draw trips a separator guard and turns a +#: property about key *content* into one about key *validity*. +_DCC_STRATEGY = st.text( + alphabet=st.characters(whitelist_categories=("L", "N")), min_size=1, max_size=8 +) +_LOCAL_ID_STRATEGY = st.text( + alphabet=st.characters(whitelist_categories=("L", "N")), min_size=1, max_size=32 +) +_MD5_STRATEGY = st.text(alphabet="abcdef0123456789", min_size=32, max_size=32) +_ARTIFACT_KIND_STRATEGY = st.sampled_from(list(ArtifactKind)) +_VERSION_STRATEGY = st.integers(min_value=0, max_value=9_999) + +#: Processor identities drawn from the vocabulary the shipped ids use +#: (letters, digits, and the ``-``/``_``/``.`` joiners), excluding the +#: values ``normalize_processor_id`` reserves. +_PROCESSOR_ID_STRATEGY = st.text( + alphabet=st.characters( + whitelist_categories=("L", "N"), whitelist_characters="-_." + ), + min_size=1, + max_size=24, +).filter( + lambda value: value.strip() == value + and value not in (".", "..") + and value not in {kind.value for kind in ArtifactKind} +) + def test_normalize_dcc_should_strip_whitespace_and_lowercase(): """Test that normalize_dcc canonicalizes its input. @@ -163,6 +194,180 @@ def test_normalize_local_id_should_raise_when_empty(self): normalize_local_id("") +class TestNormalizeProcessorId: + def test_normalize_processor_id_should_strip_whitespace_and_preserve_case(self): + """Test that normalize_processor_id trims but preserves case. + + Given: + A processor id with surrounding whitespace and mixed case. + When: + normalize_processor_id is called. + Then: + It should return the trimmed value with case preserved, so + two class-name defaults differing only in case stay distinct. + """ + # Act + result = normalize_processor_id(" TabixIntervalProcessor ") + + # Assert + assert result == "TabixIntervalProcessor" + + def test_normalize_processor_id_should_raise_when_processor_id_contains_forward_slash( + self, + ): + """Test that forward slashes are rejected as a key-shape guard. + + Given: + A processor id containing ``/``. + When: + normalize_processor_id is called. + Then: + It should raise ValueError matching "forbidden chars" so the + id cannot silently add segments to the cache key. + """ + # Act & assert + with pytest.raises(ValueError, match="forbidden chars"): + normalize_processor_id("tabix/interval") + + def test_normalize_processor_id_should_raise_when_processor_id_contains_backslash( + self, + ): + """Test that backslashes are rejected. + + Given: + A processor id containing ``\\``. + When: + normalize_processor_id is called. + Then: + It should raise ValueError so Windows-style path separators + cannot escape into cache paths. + """ + # Act & assert + with pytest.raises(ValueError): + normalize_processor_id("tabix\\interval") + + def test_normalize_processor_id_should_raise_when_processor_id_contains_null_byte( + self, + ): + """Test that null bytes are rejected. + + Given: + A processor id containing ``\\x00``. + When: + normalize_processor_id is called. + Then: + It should raise ValueError so an embedded null cannot reach a + cache path or a shell pipeline argument. + """ + # Act & assert + with pytest.raises(ValueError): + normalize_processor_id("tabix\x00interval") + + def test_normalize_processor_id_should_raise_when_empty(self): + """Test that an empty processor id is rejected. + + Given: + An empty string. + When: + normalize_processor_id is called. + Then: + It should raise ValueError, because an absent identity is + exactly the aliasing the segment exists to prevent. + """ + # Act & assert + with pytest.raises(ValueError, match="processor_id"): + normalize_processor_id("") + + def test_normalize_processor_id_should_raise_when_whitespace_only(self): + """Test that a whitespace-only processor id is rejected. + + Given: + A string of spaces. + When: + normalize_processor_id is called. + Then: + It should raise ValueError — the strip runs before the empty + check, so blanks cannot collapse the identity segment. + """ + # Act & assert + with pytest.raises(ValueError, match="processor_id"): + normalize_processor_id(" ") + + @pytest.mark.parametrize("traversal", [".", ".."]) + def test_normalize_processor_id_should_raise_when_path_traversal(self, traversal): + """Test that traversal segments are rejected at derivation. + + Given: + A processor id of "." or "..", which traverses a path segment + without containing a separator. + When: + normalize_processor_id is called. + Then: + It should raise ValueError, so the value fails here rather + than later and elsewhere, in the cache backend's own key + validation deep inside a running workflow. + """ + # Act & assert + with pytest.raises(ValueError, match="traversal"): + normalize_processor_id(traversal) + + @pytest.mark.parametrize("kind", [kind.value for kind in ArtifactKind]) + def test_normalize_processor_id_should_raise_when_it_collides_with_artifact_kind( + self, kind + ): + """Test that an identity cannot impersonate an artifact kind. + + Given: + A processor id equal to an ArtifactKind value. + When: + normalize_processor_id is called. + Then: + It should raise ValueError. Such an id would place an + artifact-kind string in the identity segment, letting an + over-specified purge prefix strip a live key down to + something is_legacy_cache_key would claim. + """ + # Act & assert + with pytest.raises(ValueError, match="artifact kind"): + normalize_processor_id(kind) + + def test_normalize_processor_id_should_raise_when_padding_hides_a_forbidden_char( + self, + ): + """Test that stripping is not a route around the character check. + + Given: + A processor id whose forbidden separator is surrounded by + whitespace. + When: + normalize_processor_id is called. + Then: + It should raise ValueError — the strip removes padding, not + the violation inside it. + """ + # Act & assert + with pytest.raises(ValueError, match="forbidden chars"): + normalize_processor_id(" a/b ") + + def test_normalize_processor_id_should_keep_two_case_variants_distinct(self): + """Test that case preservation separates two similar identities. + + Given: + Two processor ids differing only in letter case. + When: + normalize_processor_id is called on each. + Then: + It should return two different values, so case-folding can + never merge one processor's cache with another's. + """ + # Act + mixed = normalize_processor_id("BedProcessor") + upper = normalize_processor_id("BEDProcessor") + + # Assert + assert mixed != upper + + class TestWorkflowKey: def test_workflow_key_should_return_normalized_slash_joined_key(self): """Test that workflow_key returns the documented segment layout. @@ -295,24 +500,30 @@ def test_workflow_key_should_be_deterministic(self, dcc, local_id, md5, version) class TestCacheKey: - def test_cache_key_should_return_artifact_scoped_key(self): - """Test that cache_key places artifact_kind in its own segment. + def test_cache_key_should_return_artifact_and_processor_scoped_key(self): + """Test that cache_key gives artifact_kind and processor own segments. Given: - Valid inputs including an explicit ``ArtifactKind``. + Valid inputs including an explicit ``ArtifactKind`` and a + processor identity. When: cache_key is called. Then: - It should return - ``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}``. + It should return ``{dcc}/{local_id}/{artifact_kind}/ + {processor_id}/{md5}-v{processor_version}``. """ # Act key = cache_key( - "ENCODE", "ENCFF123ABC", ArtifactKind.DATA, _FIXTURE_MD5_UPPER, 2 + "ENCODE", + "ENCFF123ABC", + ArtifactKind.DATA, + _FIXTURE_MD5_UPPER, + "tabix-interval", + 2, ) # Assert - assert key == f"encode/ENCFF123ABC/data/{FIXTURE_MD5}-v2" + assert key == f"encode/ENCFF123ABC/data/tabix-interval/{FIXTURE_MD5}-v2" def test_cache_key_should_differ_between_artifact_kinds(self): """Test that data and index artifact keys are distinct. @@ -326,12 +537,37 @@ def test_cache_key_should_differ_between_artifact_kinds(self): artifact kinds never alias. """ # Act - data_key = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, 0) - index_key = cache_key("encode", "x", ArtifactKind.INDEX, FIXTURE_MD5, 0) + data_key = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "p", 0) + index_key = cache_key("encode", "x", ArtifactKind.INDEX, FIXTURE_MD5, "p", 0) # Assert assert data_key != index_key + def test_cache_key_should_differ_between_processors_at_equal_version(self): + """Test that the processor identity alone separates two processors. + + Given: + Two calls for the same file and artifact kind at the same + ``processor_version``, differing only in ``processor_id`` — + the shape that let ``TabixIntervalProcessor`` and + ``BamIndexProcessor`` (both at version 2) alias. + When: + cache_key is called for each. + Then: + It should return distinct keys, so neither processor can read + back the other's artifacts as a cache hit. + """ + # Act + tabix = cache_key( + "encode", "x", ArtifactKind.INDEX, FIXTURE_MD5, "tabix-interval", 2 + ) + bedpe = cache_key( + "encode", "x", ArtifactKind.INDEX, FIXTURE_MD5, "bedpe-interval", 2 + ) + + # Assert + assert tabix != bedpe + def test_cache_key_should_raise_when_md5_empty(self): """Test that cache_key rejects an empty md5. @@ -344,7 +580,22 @@ def test_cache_key_should_raise_when_md5_empty(self): """ # Act & assert with pytest.raises(ValueError, match="md5"): - cache_key("encode", "x", ArtifactKind.DATA, "", 0) + cache_key("encode", "x", ArtifactKind.DATA, "", "p", 0) + + def test_cache_key_should_raise_when_processor_id_empty(self): + """Test that cache_key rejects a missing processor identity. + + Given: + An empty string supplied as ``processor_id``. + When: + cache_key is called. + Then: + It should raise ValueError rather than mint a key with a + collapsed identity segment. + """ + # Act & assert + with pytest.raises(ValueError, match="processor_id"): + cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "", 0) def test_cache_key_should_raise_when_processor_version_negative(self): """Test that cache_key rejects a negative processor version. @@ -358,7 +609,7 @@ def test_cache_key_should_raise_when_processor_version_negative(self): """ # Act & assert with pytest.raises(ValueError, match="processor_version"): - cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, -1) + cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "p", -1) def test_cache_key_should_version_distinctly(self): """Test that bumping processor_version yields a distinct key. @@ -372,8 +623,370 @@ def test_cache_key_should_version_distinctly(self): trigger re-processing without purge. """ # Act - v0 = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, 0) - v1 = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, 1) + v0 = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "p", 0) + v1 = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "p", 1) # Assert assert v0 != v1 + + def test_cache_key_should_trim_the_processor_id_segment(self): + """Test that a padded identity addresses the same artifact. + + Given: + Two calls identical except that one's processor_id carries + surrounding whitespace. + When: + cache_key is called for each. + Then: + It should return identical keys, so a stray space in a + declared identity cannot fork a processor's cache in two. + """ + # Act + padded = cache_key( + "encode", "x", ArtifactKind.DATA, FIXTURE_MD5, " tabix-interval ", 2 + ) + clean = cache_key( + "encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "tabix-interval", 2 + ) + + # Assert + assert padded == clean + + def test_cache_key_should_raise_when_processor_id_contains_separator(self): + """Test that a separator in the identity cannot add a segment. + + Given: + A processor_id containing a forward slash. + When: + cache_key is called. + Then: + It should raise ValueError rather than emit a six-segment + key, so the identity can never restructure the key shape. + """ + # Act & assert + with pytest.raises(ValueError, match="forbidden chars"): + cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "tabix/oops", 2) + + def test_cache_key_should_differ_between_case_variant_processor_ids(self): + """Test that identity case survives the whole derivation path. + + Given: + Two calls identical except for the case of processor_id. + When: + cache_key is called for each. + Then: + It should return distinct keys, so two processors whose names + differ only in case never alias. + """ + # Act + mixed = cache_key( + "encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "BedProcessor", 2 + ) + upper = cache_key( + "encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "BEDProcessor", 2 + ) + + # Assert + assert mixed != upper + + def test_cache_key_should_not_confuse_the_identity_with_the_version_suffix(self): + """Test that the identity and version segments stay separable. + + Given: + Two calls at the same version, one with processor_id "p" and + the other with "p-v1" — a value that resembles an identity + with a version already folded into it. + When: + cache_key is called for each. + Then: + It should return distinct keys, so the identity segment and + the version suffix cannot be read as one another. + """ + # Act + plain = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "p", 1) + suffixed = cache_key("encode", "x", ArtifactKind.DATA, FIXTURE_MD5, "p-v1", 1) + + # Assert + assert plain != suffixed + + @settings(max_examples=50) + @given( + dcc=_DCC_STRATEGY, + local_id=_LOCAL_ID_STRATEGY, + md5=_MD5_STRATEGY, + artifact_kind=_ARTIFACT_KIND_STRATEGY, + processor_id=_PROCESSOR_ID_STRATEGY, + version=_VERSION_STRATEGY, + ) + def test_cache_key_should_be_deterministic( + self, dcc, local_id, md5, artifact_kind, processor_id, version + ): + """Test that cache_key is a pure function of its inputs. + + Given: + Any valid combination of dcc, local_id, artifact_kind, md5, + processor_id, and processor_version. + When: + cache_key is called twice with those same inputs. + Then: + It should return identical keys of exactly five slash-separated + segments, pinning the shape as well as the purity. + """ + # Act + a = cache_key(dcc, local_id, artifact_kind, md5, processor_id, version) + b = cache_key(dcc, local_id, artifact_kind, md5, processor_id, version) + + # Assert + assert a == b + assert len(a.split("/")) == 5 + + @settings(max_examples=50) + @given( + dcc=_DCC_STRATEGY, + local_id=_LOCAL_ID_STRATEGY, + md5=_MD5_STRATEGY, + artifact_kind=_ARTIFACT_KIND_STRATEGY, + first_id=_PROCESSOR_ID_STRATEGY, + second_id=_PROCESSOR_ID_STRATEGY, + version=_VERSION_STRATEGY, + ) + def test_cache_key_should_separate_any_two_distinct_processor_ids( + self, dcc, local_id, md5, artifact_kind, first_id, second_id, version + ): + """Test that the identity segment separates processors universally. + + Given: + Any two distinct processor identities, with the file, artifact + kind, and processor version held identical between them. + When: + cache_key is called for each. + Then: + It should return distinct keys across the whole input domain, + so the collision this issue closes is impossible rather than + merely absent from the two ids a single example samples. + """ + # Arrange + assume(normalize_processor_id(first_id) != normalize_processor_id(second_id)) + + # Act + first = cache_key(dcc, local_id, artifact_kind, md5, first_id, version) + second = cache_key(dcc, local_id, artifact_kind, md5, second_id, version) + + # Assert + assert first != second + + +class TestIsLegacyCacheKey: + def test_is_legacy_cache_key_should_return_true_for_retired_shape(self): + """Test that a pre-#109 key is recognised as legacy. + + Given: + A four-segment key with no processor identity, of the shape + the pipeline minted before the identity segment existed. + When: + is_legacy_cache_key is called. + Then: + It should return True so the purge sweep claims it. + """ + # Act & assert + assert is_legacy_cache_key(f"encode/ENCFF732YBO/index/{FIXTURE_MD5}-v2") is True + + def test_is_legacy_cache_key_should_return_false_for_current_shape(self): + """Test that a key carrying a processor identity is not legacy. + + Given: + A key derived by the current ``cache_key``. + When: + is_legacy_cache_key is called. + Then: + It should return False, so a live artifact is never swept. + """ + # Arrange + key = cache_key( + "encode", "ENCFF732YBO", ArtifactKind.INDEX, FIXTURE_MD5, "tabix-interval", 2 + ) + + # Act & assert + assert is_legacy_cache_key(key) is False + + @pytest.mark.parametrize( + "key", + [ + pytest.param("encode/ENCFF1/index/notes.txt", id="leaf-not-content-address"), + pytest.param(f"encode//index/{FIXTURE_MD5}-v2", id="blank-local-id"), + pytest.param(f"/ENCFF1/index/{FIXTURE_MD5}-v2", id="blank-leading-segment"), + pytest.param("", id="empty-string"), + pytest.param(f"encode/ENCFF1/{FIXTURE_MD5}-v2", id="three-segments"), + pytest.param( + f"encode/ENCFF1/index/tabix/extra/{FIXTURE_MD5}-v2", id="six-segments" + ), + pytest.param(f"encode/ENCFF1/index/{FIXTURE_MD5}-v2/", id="trailing-slash"), + pytest.param("encode/ENCFF1/index/", id="s3-directory-marker"), + pytest.param( + f"encode/ENCFF1/index/{_FIXTURE_MD5_UPPER}-v2", id="uppercase-md5-leaf" + ), + pytest.param(f"encode/ENCFF1/index/{FIXTURE_MD5[:31]}-v2", id="short-md5"), + pytest.param(f"encode/ENCFF1/index/{FIXTURE_MD5}", id="no-version-suffix"), + pytest.param(f"encode/ENCFF1/index/{FIXTURE_MD5}-v", id="empty-version"), + pytest.param(f"encode/ENCFF1/index/{FIXTURE_MD5}-v2.tbi", id="leaf-suffix"), + pytest.param(f"encode/ENCFF1/index/{FIXTURE_MD5}-v2 ", id="trailing-space"), + ], + ) + def test_is_legacy_cache_key_should_return_false_for_unclaimable_keys(self, key): + """Test that the sweep never claims a key it does not own. + + Given: + A key that is malformed, foreign, or of the wrong segment + count — the shapes an unrelated object sharing the bucket + could take. + When: + is_legacy_cache_key is called. + Then: + It should return False. This predicate gates an irreversible + delete, so a false positive is data loss while a false + negative only leaves a stale object behind. + """ + # Act & assert + assert is_legacy_cache_key(key) is False + + @pytest.mark.parametrize( + "key", + [ + pytest.param(f"logs/2024/01/{FIXTURE_MD5}-v2", id="foreign-object"), + pytest.param(f"encode/ENCFF1/wat/{FIXTURE_MD5}-v2", id="not-an-artifact-kind"), + ], + ) + def test_is_legacy_cache_key_should_require_a_real_artifact_kind(self, key): + """Test that a four-segment shape alone does not make a key ours. + + Given: + A four-segment key with a content-addressed leaf whose third + segment is not an ArtifactKind — an unrelated object that + happens to match the retired scheme's shape. + When: + is_legacy_cache_key is called. + Then: + It should return False. Every retired key carried a real + artifact kind by construction, so requiring one costs no true + positive and keeps the sweep off objects it does not own. + """ + # Act & assert + assert is_legacy_cache_key(key) is False + + def test_is_legacy_cache_key_should_return_false_for_an_over_stripped_current_key( + self, + ): + """Test that a mis-prefixed live key is never claimed. + + Given: + A current five-segment key with its leading dcc segment + removed, exactly as ``purge_s3`` produces when handed a + prefix carrying one segment too many. + When: + is_legacy_cache_key is called. + Then: + It should return False, because the processor identity lands + in the artifact-kind slot and fails that check. Without it a + single mistyped WORKFLOW_S3_PREFIX would delete a live cache. + """ + # Arrange + live = cache_key( + "encode", "ENCFF1", ArtifactKind.INDEX, FIXTURE_MD5, "tabix-interval", 2 + ) + + # Act & assert + assert is_legacy_cache_key(live.split("/", 1)[1]) is False + + def test_is_legacy_cache_key_should_return_false_for_a_workflow_key(self): + """Test that the mutex namespace is not swept. + + Given: + A key produced by workflow_key, which is also four segments + but carries a bare ``v{n}`` leaf. + When: + is_legacy_cache_key is called. + Then: + It should return False, so a mutex key stored alongside the + cache is never mistaken for a retired artifact. + """ + # Arrange + mutex = workflow_key("encode", "ENCFF1", FIXTURE_MD5, 1) + + # Act & assert + assert is_legacy_cache_key(mutex) is False + + @pytest.mark.parametrize("version", ["v0", "v10", "v01"]) + def test_is_legacy_cache_key_should_claim_any_non_negative_version(self, version): + """Test that the whole retired version range is reclaimed. + + Given: + Legacy-shaped keys whose leaf carries a single-digit, + multi-digit, or zero-padded version. + When: + is_legacy_cache_key is called. + Then: + It should return True for each, so no corner of the retired + population is left behind as unreachable storage cost. + """ + # Act & assert + assert is_legacy_cache_key(f"encode/ENCFF1/index/{FIXTURE_MD5}-{version}") + + @settings(max_examples=50) + @given( + dcc=_DCC_STRATEGY, + local_id=_LOCAL_ID_STRATEGY, + md5=_MD5_STRATEGY, + artifact_kind=_ARTIFACT_KIND_STRATEGY, + processor_id=_PROCESSOR_ID_STRATEGY, + version=_VERSION_STRATEGY, + ) + def test_is_legacy_cache_key_should_never_claim_a_derived_key( + self, dcc, local_id, md5, artifact_kind, processor_id, version + ): + """Test that no key the pipeline can mint is ever sweepable. + + Given: + Any key derived by cache_key from any valid inputs. + When: + is_legacy_cache_key is called on it. + Then: + It should always return False. This is the safety property + that makes ``purge --apply`` sound: whatever the live + pipeline writes, the sweep cannot delete it. + """ + # Act + derived = cache_key(dcc, local_id, artifact_kind, md5, processor_id, version) + + # Assert + assert is_legacy_cache_key(derived) is False + + @settings(max_examples=50) + @given( + dcc=_DCC_STRATEGY, + local_id=_LOCAL_ID_STRATEGY, + md5=_MD5_STRATEGY, + artifact_kind=_ARTIFACT_KIND_STRATEGY, + version=_VERSION_STRATEGY, + ) + def test_is_legacy_cache_key_should_claim_every_retired_key( + self, dcc, local_id, md5, artifact_kind, version + ): + """Test that the sweep reclaims the entire retired population. + + Given: + Any key assembled in the retired four-segment scheme from + valid components. + When: + is_legacy_cache_key is called on it. + Then: + It should always return True, so the migration leaves no + orphaned artifact paying storage cost forever. + """ + # Arrange + retired = ( + f"{normalize_dcc(dcc)}/{normalize_local_id(local_id)}/" + f"{artifact_kind.value}/{normalize_md5(md5)}-v{version}" + ) + + # Act & assert + assert is_legacy_cache_key(retired) is True diff --git a/tests/test_workflows/test_processors_bam.py b/tests/test_workflows/test_processors_bam.py index 2813742..bd6ad58 100644 --- a/tests/test_workflows/test_processors_bam.py +++ b/tests/test_workflows/test_processors_bam.py @@ -138,6 +138,22 @@ async def _async_false() -> bool: class TestBamIndexProcessor: + def test_processor_id_should_be_the_pinned_literal(self): + """Test that the shipped identity is exactly "bam-index". + + Given: + The shipped BamIndexProcessor class. + When: + processor_id is read off it. + Then: + It should be exactly "bam-index". The literal is asserted + rather than derived because it is a wire constant — every + cached BAI is keyed under it, so changing the string silently + invalidates the processor's whole cached corpus. + """ + # Act & assert + assert BamIndexProcessor.processor_id == "bam-index" + def test_needs_processing_should_accept_bam(self): """Test that the processor claims BAM inputs. @@ -352,6 +368,7 @@ async def test_run_should_skip_index_step_when_index_already_cached( local_id="ENCFF123", artifact_kind=ArtifactKind.INDEX, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) cache = LocalFsCache(cache_root) @@ -463,6 +480,7 @@ async def test_run_should_skip_sort_when_data_artifact_cached_for_sam( local_id="ENCFF123", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) cache = LocalFsCache(cache_root) diff --git a/tests/test_workflows/test_processors_base.py b/tests/test_workflows/test_processors_base.py index 87bd44d..db7f22a 100644 --- a/tests/test_workflows/test_processors_base.py +++ b/tests/test_workflows/test_processors_base.py @@ -131,7 +131,197 @@ async def run(self, file_meta, workdir, cache): # Assert assert kinds == (ArtifactKind.INDEX,) - def test_cache_key_for_should_match_keys_module_with_processor_version(self): + def test_processor_id_should_default_to_class_name_when_undeclared(self): + """Test that a subclass without an explicit id gets its class name. + + Given: + A Processor subclass that declares no ``processor_id``. + When: + The attribute is read off the class. + Then: + It should be the class's own name, so a processor author who + forgets still derives keys distinct from every other + processor's rather than silently aliasing them. + """ + # Act & assert + assert _ConcreteProcessor.processor_id == "_ConcreteProcessor" + + def test_processor_id_should_preserve_an_explicitly_declared_value(self): + """Test that a declared processor_id survives the default. + + Given: + A Processor subclass declaring ``processor_id`` in its body. + When: + The attribute is read off the class. + Then: + It should keep the declared value, so the identity — and + therefore every artifact keyed under it — survives a rename + of the class. + """ + + # Arrange + class _Pinned(Processor): + processor_id = "pinned-identity" + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + # Act & assert + assert _Pinned.processor_id == "pinned-identity" + + def test_processor_id_should_default_to_class_name_when_declared_empty(self): + """Test that a blank declaration is treated as no declaration. + + Given: + A Processor subclass declaring ``processor_id = ""``. + When: + The attribute is read off the class. + Then: + It should be the class's own name — an empty identity is the + same failure as a missing one, so it takes the same safe + default rather than collapsing the key segment. + """ + + # Arrange + class _Blank(Processor): + processor_id = "" + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + # Act & assert + assert _Blank.processor_id == "_Blank" + + def test_processor_id_should_raise_when_declared_whitespace_only(self): + """Test that a malformed identity fails when the class is written. + + Given: + A Processor subclass declaring a whitespace-only + ``processor_id`` — truthy, so it is not treated as absent. + When: + The class is declared. + Then: + It should raise ValueError at definition time, so the mistake + surfaces on import rather than per-request inside a worker, + far from the class that caused it. + """ + # Act & assert + with pytest.raises(ValueError, match="processor_id"): + + class _Blank(Processor): + processor_id = " " + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + def test_processor_id_should_raise_when_declared_id_collides_with_artifact_kind( + self, + ): + """Test that a processor cannot take an artifact kind as its name. + + Given: + A Processor subclass declaring ``processor_id = "index"``. + When: + The class is declared. + Then: + It should raise ValueError. Such an identity would let an + over-specified purge prefix reduce this processor's live keys + to something indistinguishable from a retired one. + """ + # Act & assert + with pytest.raises(ValueError, match="artifact kind"): + + class _KindNamed(Processor): + processor_id = ArtifactKind.INDEX.value + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + def test_processor_id_should_be_distinct_at_every_level_of_a_hierarchy(self): + """Test that no two levels of an inheritance chain share an identity. + + Given: + A three-level chain of Processor subclasses, none declaring + an identity. + When: + Each class's ``processor_id`` is read. + Then: + Each should carry its own class name, so a deep hierarchy + cannot alias any two of its members' caches. + """ + + # Arrange + class _Level1(Processor): + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + class _Level2(_Level1): + pass + + class _Level3(_Level2): + pass + + # Act + ids = {_Level1.processor_id, _Level2.processor_id, _Level3.processor_id} + + # Assert + assert ids == {"_Level1", "_Level2", "_Level3"} + + def test_processor_id_should_ignore_a_value_supplied_by_a_mixin(self): + """Test that only the class's own body can declare an identity. + + Given: + A plain mixin declaring ``processor_id``, and a Processor + subclass inheriting from that mixin. + When: + The subclass's attribute is read. + Then: + It should be the subclass's class name, not the mixin's + value — the default is applied from the class's own + ``__dict__`` rather than the MRO, so factoring a pinned + identity into a mixin silently loses it. + """ + + # Arrange + class _IdentityMixin: + processor_id = "from-mixin" + + class _Mixed(_IdentityMixin, Processor): + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + # Act & assert + assert _Mixed.processor_id == "_Mixed" + + def test_processor_id_should_not_be_inherited_by_a_subclass(self): + """Test that subclassing a pinned processor mints a fresh identity. + + Given: + A subclass of a processor that declares its own + ``processor_id``. + When: + The subclass's attribute is read. + Then: + It should be the subclass's class name rather than the + inherited value — the subclass may emit different bytes at + the same processor_version, which is exactly the aliasing the + identity prevents. + """ + + # Arrange + class _Base(Processor): + processor_id = "base-identity" + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + class _Derived(_Base): + pass + + # Act & assert + assert _Derived.processor_id == "_Derived" + + def test_cache_key_for_should_match_keys_module_with_processor_identity(self): """Test that cache_key_for is the canonical key the router probes. Given: @@ -140,7 +330,7 @@ def test_cache_key_for_should_match_keys_module_with_processor_version(self): cache_key_for is called for the DATA artifact. Then: It should equal the key keys.cache_key derives with the same - identity and the processor's version — so the router probe, + identity, processor id, and version — so the router probe, the processor put, and the StageComplete event all agree. """ # Arrange @@ -159,9 +349,120 @@ def test_cache_key_for_should_match_keys_module_with_processor_version(self): local_id="ENCFF1", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id="_ConcreteProcessor", processor_version=1, ) + def test_cache_key_for_should_differ_between_processors_at_equal_version(self): + """Test that two processors never derive the same artifact key. + + Given: + Two Processor subclasses at the same ``processor_version``, + both asked for the INDEX artifact of the same file — the + shape that would let one serve the other's artifact as a + cache hit. + When: + cache_key_for is called on each. + Then: + It should return distinct keys, so the collision is + impossible rather than merely absent from today's registry. + """ + + # Arrange + class _FirstProcessor(Processor): + processor_version = 2 + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + class _SecondProcessor(Processor): + processor_version = 2 + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + meta = { + "dcc": {"dcc_abbreviation": "ENCODE"}, + "local_id": "ENCFF1", + "md5": FIXTURE_MD5, + } + + # Act + first = _FirstProcessor().cache_key_for(meta, ArtifactKind.INDEX) + second = _SecondProcessor().cache_key_for(meta, ArtifactKind.INDEX) + + # Assert + assert first != second + + def test_cache_key_for_should_place_the_identity_in_its_own_segment(self): + """Test that the identity actually occupies the identity slot. + + Given: + A processor whose identity was defaulted to its class name + and a complete file_meta. + When: + cache_key_for is called for the DATA artifact and the result + is split on "/". + Then: + The fourth segment should be the processor's identity, so the + key shape the purge sweep reasons about is the one the + processor actually writes under. + """ + # Arrange + meta = { + "dcc": {"dcc_abbreviation": "ENCODE"}, + "local_id": "ENCFF1", + "md5": FIXTURE_MD5, + } + + # Act + segments = _ConcreteProcessor().cache_key_for(meta, ArtifactKind.DATA).split("/") + + # Assert + assert segments[3] == _ConcreteProcessor.processor_id + + def test_cache_key_for_should_alias_when_two_processors_declare_one_identity(self): + """Test the honest boundary of the base class's separation guarantee. + + Given: + Two Processor subclasses that both declare the *same* + explicit identity, for the same file and artifact kind. + When: + cache_key_for is called on each. + Then: + It should return equal keys. The class-name default is a + safety net for processors that declare nothing, not a + guarantee — enforcement of uniqueness lives in + ``ProcessorRegistry.register``, and this pins where the + responsibility actually sits. + """ + + # Arrange + class _FirstDeclared(Processor): + processor_id = "shared-identity" + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + class _SecondDeclared(Processor): + processor_id = "shared-identity" + + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + meta = { + "dcc": {"dcc_abbreviation": "ENCODE"}, + "local_id": "ENCFF1", + "md5": FIXTURE_MD5, + } + + # Act + first = _FirstDeclared().cache_key_for(meta, ArtifactKind.INDEX) + second = _SecondDeclared().cache_key_for(meta, ArtifactKind.INDEX) + + # Assert + assert first == second + def test_cache_key_for_should_raise_when_file_meta_incomplete(self): """Test that cache_key_for surfaces incomplete metadata loudly. diff --git a/tests/test_workflows/test_processors_passthrough.py b/tests/test_workflows/test_processors_passthrough.py index ed81237..3ac126e 100644 --- a/tests/test_workflows/test_processors_passthrough.py +++ b/tests/test_workflows/test_processors_passthrough.py @@ -8,6 +8,22 @@ class TestPassthroughProcessor: + def test_processor_id_should_be_the_pinned_literal(self): + """Test that the shipped identity is exactly "passthrough". + + Given: + The shipped PassthroughProcessor class. + When: + processor_id is read off it. + Then: + It should be exactly "passthrough". The literal is asserted + rather than derived because it is a wire constant — every + cached artifact is keyed under it, so changing the string + silently invalidates the processor's whole cached corpus. + """ + # Act & assert + assert PassthroughProcessor.processor_id == "passthrough" + def test_needs_processing_should_return_false_for_csv(self): """Test that PassthroughProcessor reports no work for CSV inputs. diff --git a/tests/test_workflows/test_processors_registry.py b/tests/test_workflows/test_processors_registry.py index b18af47..d87b179 100644 --- a/tests/test_workflows/test_processors_registry.py +++ b/tests/test_workflows/test_processors_registry.py @@ -5,10 +5,14 @@ from pathlib import Path from typing import Any +import pytest + from cfdb.workflows.models import ArtifactKind +from cfdb.workflows.processors.bam import BamIndexProcessor from cfdb.workflows.processors.base import Processor from cfdb.workflows.processors.passthrough import PassthroughProcessor from cfdb.workflows.processors.registry import ProcessorRegistry, default_registry +from cfdb.workflows.processors.tabix import TabixIntervalProcessor class _BamOnly(Processor): @@ -25,6 +29,71 @@ async def run( return {} +#: A near-twin of ``_BamOnly``. It exists because the duplicate-identity +#: guard rejects two instances of one class, so the "first registration +#: wins" contract needs two *distinct* classes claiming BAM to be +#: exercised at all. +class _BamAlternate(Processor): + processor_version = 0 + supported_formats = frozenset({"BAM"}) + artifact_kinds = (ArtifactKind.DATA, ArtifactKind.INDEX) + + async def run( + self, + file_meta: dict[str, Any], + workdir: Path, + cache_root: Path, + ) -> dict[str, str]: + return {} + + +#: Two distinct classes pinning one identity — the collision the +#: class-name default cannot catch, and the one the registry must. +class _PinnedIncumbent(Processor): + processor_id = "pinned-identity" + processor_version = 0 + supported_formats = frozenset({"BAM"}) + artifact_kinds = (ArtifactKind.INDEX,) + + async def run( + self, + file_meta: dict[str, Any], + workdir: Path, + cache_root: Path, + ) -> dict[str, str]: + return {} + + +class _PinnedNewcomer(Processor): + processor_id = "pinned-identity" + processor_version = 0 + supported_formats = frozenset({"BAM"}) + artifact_kinds = (ArtifactKind.INDEX,) + + async def run( + self, + file_meta: dict[str, Any], + workdir: Path, + cache_root: Path, + ) -> dict[str, str]: + return {} + + +class _PinnedDisjointFormats(Processor): + processor_id = "pinned-identity" + processor_version = 0 + supported_formats = frozenset({"VCF"}) + artifact_kinds = (ArtifactKind.INDEX,) + + async def run( + self, + file_meta: dict[str, Any], + workdir: Path, + cache_root: Path, + ) -> dict[str, str]: + return {} + + class _TabixOnly(Processor): processor_version = 0 supported_formats = frozenset({"VCF", "BED"}) @@ -106,16 +175,19 @@ def test_lookup_for_should_honor_registration_order_when_duplicate_support(self) """Test that the first registered processor wins when both match. Given: - Two processors both claiming BAM, registered in a known order. + Two distinct processors both claiming BAM, registered in a + known order. When: lookup_for is called with a BAM file. Then: - It should return the processor registered first, matching the - documented "first match wins" contract. + It should accept both registrations and return the processor + registered first — the duplicate-identity guard is scoped to + ``processor_id`` alone, so two distinct processors claiming + one format remain legal and order-resolved. """ # Arrange first = _BamOnly() - second = _BamOnly() + second = _BamAlternate() registry = ProcessorRegistry() registry.register(first) registry.register(second) @@ -126,6 +198,98 @@ def test_lookup_for_should_honor_registration_order_when_duplicate_support(self) # Assert assert result is first + def test_register_should_raise_when_two_classes_declare_one_processor_id(self): + """Test that the registry refuses two processors sharing an identity. + + Given: + A registry holding a processor, and a processor of a + different class that declares the same ``processor_id`` — the + realistic collision, since a new processor copy-pasting a + pinned identity is the one case the class-name default cannot + catch. + When: + register is called for the second. + Then: + It should raise ValueError naming the duplicated identity and + the incumbent's class, so an operator can find the conflict + without reading the registry. + """ + # Arrange + registry = ProcessorRegistry() + registry.register(_PinnedIncumbent()) + + # Act & assert + with pytest.raises(ValueError, match=r"'pinned-identity'.*_PinnedIncumbent"): + registry.register(_PinnedNewcomer()) + + def test_register_should_raise_when_the_same_processor_class_registers_twice(self): + """Test that re-registering one processor is rejected too. + + Given: + A registry already holding a processor, and a second instance + of that same class. + When: + register is called for the second. + Then: + It should raise ValueError — registration is not idempotent, + so double-wiring a registry is caught rather than silently + duplicating a lookup candidate. + """ + # Arrange + registry = ProcessorRegistry() + registry.register(_BamOnly()) + + # Act & assert + with pytest.raises(ValueError, match="processor_id"): + registry.register(_BamOnly()) + + def test_register_should_reject_a_duplicate_identity_across_disjoint_formats(self): + """Test that disjoint formats are not an escape from the guard. + + Given: + Two processors sharing an identity but claiming completely + different ``supported_formats``. + When: + register is called for the second. + Then: + It should still raise ValueError. Cache keys are scoped by + identity and artifact kind, not by format, so disjoint + formats do not prevent the two from aliasing. + """ + # Arrange + registry = ProcessorRegistry() + registry.register(_PinnedIncumbent()) + + # Act & assert + with pytest.raises(ValueError, match="processor_id"): + registry.register(_PinnedDisjointFormats()) + + def test_lookup_for_should_resolve_to_the_incumbent_after_a_rejected_register(self): + """Test that a rejected registration leaves the registry unchanged. + + Given: + A registry whose second register call raised on a duplicate + identity. + When: + lookup_for is called for a format the rejected processor also + claimed. + Then: + It should return the incumbent, so a failed registration + leaves no partial state behind for lookup to trip over. + """ + # Arrange + registry = ProcessorRegistry() + incumbent = _PinnedIncumbent() + registry.register(incumbent) + with pytest.raises(ValueError): + registry.register(_PinnedNewcomer()) + + # Act + result = registry.lookup_for({"file_format": {"name": "BAM"}}) + + # Assert + assert result is incumbent + class TestDefaultRegistry: def test_default_registry_should_include_passthrough_processor(self): @@ -144,3 +308,62 @@ def test_default_registry_should_include_passthrough_processor(self): # Assert assert isinstance(result, PassthroughProcessor) + + def test_default_registry_should_accept_the_shipped_processor_wiring(self): + """Test that the production wiring survives the duplicate guard. + + Given: + A default registry and the two processors the API registers + onto it during startup. + When: + Both are registered, mirroring the application lifespan. + Then: + It should accept both and resolve each format to its + processor — proving the three shipped identities are pairwise + distinct, so shipping a colliding one fails in CI rather than + crash-looping the API at boot. + """ + # Arrange + registry = default_registry() + + # Act + registry.register(BamIndexProcessor()) + registry.register(TabixIntervalProcessor()) + + # Assert + assert isinstance( + registry.lookup_for({"file_format": {"name": "BAM"}}), BamIndexProcessor + ) + assert isinstance( + registry.lookup_for({"file_format": {"name": "BED"}}), + TabixIntervalProcessor, + ) + assert isinstance( + registry.lookup_for({"file_format": {"name": "bigWig"}}), + PassthroughProcessor, + ) + + def test_default_registry_should_return_an_independent_registry_each_call(self): + """Test that each call yields a registry with no shared state. + + Given: + Two independent calls to default_registry. + When: + A processor is registered onto the first only. + Then: + The second should still accept its own instance of that + processor — this is why a lifespan that runs twice (a reload, + a worker restart) cannot trip the duplicate guard at startup. + """ + # Arrange + first = default_registry() + first.register(BamIndexProcessor()) + + # Act + second = default_registry() + second.register(BamIndexProcessor()) + + # Assert + assert isinstance( + second.lookup_for({"file_format": {"name": "BAM"}}), BamIndexProcessor + ) diff --git a/tests/test_workflows/test_processors_tabix.py b/tests/test_workflows/test_processors_tabix.py index 0ea59db..0c99478 100644 --- a/tests/test_workflows/test_processors_tabix.py +++ b/tests/test_workflows/test_processors_tabix.py @@ -15,6 +15,7 @@ from cfdb.workflows.events import Complete from cfdb.workflows.models import ArtifactKind from cfdb.workflows.processors import tabix as tabix_module +from cfdb.workflows.processors.bam import BamIndexProcessor from cfdb.workflows.processors.tabix import TabixIntervalProcessor from tests.test_workflows import FIXTURE_MD5 @@ -264,6 +265,52 @@ def test__source_looks_processable_should_reject_any_starch_prefixed_payload(pay class TestTabixIntervalProcessor: + def test_processor_id_should_be_the_pinned_literal(self): + """Test that the shipped identity is exactly "tabix-interval". + + Given: + The shipped TabixIntervalProcessor class. + When: + processor_id is read off it. + Then: + It should be exactly "tabix-interval". The literal is + asserted rather than derived because it is a wire constant — + the README documents it inside an example key, and changing + the string silently invalidates every cached tabix artifact. + """ + # Act & assert + assert TabixIntervalProcessor.processor_id == "tabix-interval" + + def test_cache_key_for_should_differ_from_the_bam_processor_at_equal_version(self): + """Test that the two shipped processors cannot alias. + + Given: + TabixIntervalProcessor and BamIndexProcessor, which both + declare processor_version 2, and one file identity. + When: + cache_key_for is called on each for the INDEX artifact. + Then: + It should return distinct keys. This is the exact pair the + issue names as staying apart only by the accident of disjoint + supported_formats; the identity segment now separates them + whatever their formats or versions. + """ + # Arrange + meta = { + "dcc": {"dcc_abbreviation": "ENCODE"}, + "local_id": "ENCFF1", + "md5": FIXTURE_MD5, + } + + # Act + tabix = TabixIntervalProcessor().cache_key_for(meta, ArtifactKind.INDEX) + bam = BamIndexProcessor().cache_key_for(meta, ArtifactKind.INDEX) + + # Assert + assert TabixIntervalProcessor.processor_version == 2 + assert BamIndexProcessor.processor_version == 2 + assert tabix != bam + @pytest.mark.parametrize( "fmt", ["VCF", "GFF", "GFF3", "GTF", "BED", "BroadPeak", "NarrowPeak", "bigBed"], @@ -490,6 +537,7 @@ async def test_run_should_skip_stage_one_when_data_already_cached( local_id="ENCFF-VCF", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) cache = LocalFsCache(cache_root) @@ -637,6 +685,7 @@ async def zero_count(self, _bgz, _fmt): local_id="ENCFF-BED", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=TabixIntervalProcessor.processor_id, processor_version=TabixIntervalProcessor().processor_version, ) assert await cache.head(data_key) is None @@ -693,6 +742,7 @@ async def fail_run(argv): local_id=f"ENCFF-{fmt}", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=TabixIntervalProcessor.processor_id, processor_version=TabixIntervalProcessor().processor_version, ) assert await cache.head(data_key) is None @@ -785,6 +835,7 @@ async def fail_run(argv): local_id="ENCFF-bigBed", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=TabixIntervalProcessor.processor_id, processor_version=TabixIntervalProcessor().processor_version, ) assert await cache.head(data_key) is None @@ -818,6 +869,7 @@ async def test_run_should_not_serve_a_poisoned_v1_artifact( local_id="ENCFF-BED", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=TabixIntervalProcessor.processor_id, processor_version=1, ) await cache.put(v1_key, seed) @@ -844,6 +896,7 @@ async def fail_shell(cmd): local_id="ENCFF-BED", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=TabixIntervalProcessor.processor_id, processor_version=TabixIntervalProcessor().processor_version, ) assert current_key != v1_key From fe8133358e266e71ff2b4fb29843a5312f215fba Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 09:12:01 -0400 Subject: [PATCH 05/14] test: Cover the purge sweep and its CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cfdb.cli had no test module at all, and this change adds a command to it that deletes production data irrecoverably. Both usage guards are now covered — the no-target case and the ambiguous case where an S3 bucket and a local root both resolve — and the ambiguity test asserts that neither sweep ran, not merely that the message was printed. An autouse fixture clears the five environment variables the options bind to. Every one of them leaks from a developer's shell, and without the fixture the ambiguity guard would fire on tests that never mention S3, passing in CI and failing locally. The sweep's boundaries get direct coverage: pagination across more than one listing page, the delete-request batch boundary, prefixes written bare or slashed, and a neighbouring environment whose prefix is a strict textual prefix of the swept one. The over-specified prefix is a regression test — it deletes a live artifact without the artifact-kind check in is_legacy_cache_key. The DeleteObjects error path uses a client double rather than moto, which populates Errors only for versioned deletes the sweep never issues. purge_s3 takes its client as a public parameter, so the double stays on the public surface. A Hypothesis property spans the two modules: for any file identity seeded under both schemes, the sweep deletes exactly the retired entry and leaves what cache_key mints. That is what catches the deriver and the sweep drifting apart in a future key change. --- tests/test_cli.py | 376 +++++++++++ tests/test_workflows/test_purge.py | 985 +++++++++++++++++++++++++++++ 2 files changed, 1361 insertions(+) create mode 100644 tests/test_cli.py create mode 100644 tests/test_workflows/test_purge.py diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..ba956d6 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,376 @@ +"""Tests for the ``cfdb`` operator CLI.""" + +from __future__ import annotations + +import boto3 +import pytest +from click.testing import CliRunner +from moto import mock_aws + +from cfdb.cli import cli +from cfdb.workflows import purge as purge_module +from cfdb.workflows.keys import cache_key +from cfdb.workflows.models import ArtifactKind +from cfdb.workflows.purge import PurgeReport +from tests.test_workflows import FIXTURE_MD5 + +_BUCKET = "cfdb-test-cli" + +#: A key of the retired four-segment shape the sweep reclaims. +_LEGACY_KEY = f"encode/ENCFF732YBO/index/{FIXTURE_MD5}-v2" + +#: A current-scheme key, which every sweep must leave alone. +_CURRENT_KEY = cache_key( + dcc="encode", + local_id="ENCFF732YBO", + artifact_kind=ArtifactKind.INDEX, + md5=FIXTURE_MD5, + processor_id="tabix-interval", + processor_version=2, +) + +#: Every option on ``purge-legacy-cache`` is bound to one of these. They +#: are cleared for each test because an exported value on the developer's +#: machine would otherwise silently change which store is targeted — and +#: the ambiguity guard would fire on tests that never mention S3. +_BOUND_ENV_VARS = ( + "WORKFLOW_S3_BUCKET", + "WORKFLOW_S3_PREFIX", + "AWS_ENDPOINT_URL", + "AWS_REGION", + "SYNC_DATA_DIR", +) + + +@pytest.fixture(autouse=True) +def clear_bound_env(monkeypatch): + """Remove every environment variable the purge options bind to.""" + for name in _BOUND_ENV_VARS: + monkeypatch.delenv(name, raising=False) + + +@pytest.fixture() +def cache_root(tmp_path): + """Return a local cache root holding one legacy and one current entry.""" + for key in (_LEGACY_KEY, _CURRENT_KEY): + path = tmp_path / key + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b"artifact") + return tmp_path + + +def _invoke(*args): + """Run ``cfdb purge-legacy-cache`` with ``args`` and return the result.""" + return CliRunner().invoke(cli, ["purge-legacy-cache", *args]) + + +class TestPurgeLegacyCacheCommand: + def test_purge_legacy_cache_should_default_to_a_dry_run(self, cache_root): + """Test that omitting --apply destroys nothing. + + Given: + A local cache root holding one legacy and one current entry. + When: + The command is invoked with --local-root and no --apply. + Then: + It should exit 0, report the target and the one match, print + the dry-run notice, and leave both files on disk — an + operator cannot destroy a cache by forgetting a flag. + """ + # Act + result = _invoke("--local-root", str(cache_root)) + + # Assert + assert result.exit_code == 0 + assert f"Target: {cache_root}" in result.output + assert "Scanned: 2" in result.output + assert "Legacy entries: 1" in result.output + assert "Dry run" in result.output + assert (cache_root / _LEGACY_KEY).exists() + assert (cache_root / _CURRENT_KEY).exists() + + def test_purge_legacy_cache_should_delete_the_legacy_entry_when_applied( + self, cache_root + ): + """Test that --apply sweeps the local cache. + + Given: + The same local cache root. + When: + The command is invoked with --local-root and --apply. + Then: + It should exit 0, report the deletion, and remove only the + legacy entry. + """ + # Act + result = _invoke("--local-root", str(cache_root), "--apply") + + # Assert + assert result.exit_code == 0 + assert "Deleted: 1" in result.output + assert "Dry run" not in result.output + assert not (cache_root / _LEGACY_KEY).exists() + assert (cache_root / _CURRENT_KEY).exists() + + def test_purge_legacy_cache_should_refuse_when_both_stores_resolve( + self, cache_root, monkeypatch, mocker + ): + """Test that an ambiguous target purges neither store. + + Given: + WORKFLOW_S3_BUCKET set in the environment alongside an + explicit --local-root. + When: + The command is invoked with --apply. + Then: + It should exit non-zero with a usage error and call neither + sweep. Purging the wrong store is unrecoverable, so the + command must refuse rather than guess which was meant. + """ + # Arrange + monkeypatch.setenv("WORKFLOW_S3_BUCKET", "some-bucket") + local = mocker.patch.object(purge_module, "purge_local") + remote = mocker.patch.object(purge_module, "purge_s3") + + # Act + result = _invoke("--local-root", str(cache_root), "--apply") + + # Assert + assert result.exit_code != 0 + assert "Both an S3 bucket and a local cache root" in result.output + local.assert_not_called() + remote.assert_not_called() + + def test_purge_legacy_cache_should_refuse_when_no_store_resolves(self, mocker): + """Test that the command names the ways to supply a target. + + Given: + Neither --s3-bucket, --local-root, WORKFLOW_S3_BUCKET, nor + SYNC_DATA_DIR. + When: + The command is invoked. + Then: + It should exit non-zero with a usage error listing all four, + rather than silently sweeping nothing and reporting success. + """ + # Arrange + local = mocker.patch.object(purge_module, "purge_local") + + # Act + result = _invoke() + + # Assert + assert result.exit_code != 0 + assert "No cache to purge" in result.output + local.assert_not_called() + + def test_purge_legacy_cache_should_fall_back_to_the_sync_data_dir( + self, cache_root, monkeypatch, tmp_path + ): + """Test that SYNC_DATA_DIR resolves the documented default root. + + Given: + SYNC_DATA_DIR pointing at a directory whose cache/ subtree + holds a legacy entry, and no explicit target. + When: + The command is invoked with --apply. + Then: + It should sweep $SYNC_DATA_DIR/cache and name that path as + the target. + """ + # Arrange + data_dir = tmp_path / "data" + (data_dir / "cache").mkdir(parents=True) + entry = data_dir / "cache" / _LEGACY_KEY + entry.parent.mkdir(parents=True) + entry.write_bytes(b"stale") + monkeypatch.setenv("SYNC_DATA_DIR", str(data_dir)) + + # Act + result = _invoke("--apply") + + # Assert + assert result.exit_code == 0 + assert f"Target: {data_dir / 'cache'}" in result.output + assert not entry.exists() + + def test_purge_legacy_cache_should_prefer_an_explicit_root_over_the_fallback( + self, cache_root, monkeypatch, tmp_path + ): + """Test that an explicit root wins over the environment default. + + Given: + SYNC_DATA_DIR set to one directory and --local-root passed + for another. + When: + The command is invoked. + Then: + It should target the explicit root, so the flag an operator + typed beats the one their shell supplied. + """ + # Arrange + monkeypatch.setenv("SYNC_DATA_DIR", str(tmp_path / "elsewhere")) + + # Act + result = _invoke("--local-root", str(cache_root)) + + # Assert + assert result.exit_code == 0 + assert f"Target: {cache_root}" in result.output + + def test_purge_legacy_cache_should_build_no_s3_client_for_a_local_sweep( + self, cache_root, mocker + ): + """Test that the local branch never reaches for AWS. + + Given: + A local cache root, with the sweep entry points patched. + When: + The command is invoked with --local-root. + Then: + It should call purge_local and neither purge_s3 nor the + client factory — constructing a boto3 client in an + environment with no AWS configuration is a latent failure on + a path that does not need one. + """ + # Arrange + local = mocker.patch.object( + purge_module, "purge_local", return_value=PurgeReport() + ) + remote = mocker.patch.object(purge_module, "purge_s3") + factory = mocker.patch.object(purge_module, "build_s3_client") + + # Act + result = _invoke("--local-root", str(cache_root)) + + # Assert + assert result.exit_code == 0 + local.assert_called_once() + remote.assert_not_called() + factory.assert_not_called() + + def test_purge_legacy_cache_should_sweep_the_configured_bucket_and_prefix( + self, mocker + ): + """Test that the S3 branch wires bucket, prefix, and apply through. + + Given: + A moto-backed bucket holding a legacy and a current object + under a dev/ prefix. + When: + The command is invoked with --s3-bucket, --s3-prefix, and + --apply. + Then: + It should report the prefixed S3 target and delete only the + legacy object. + """ + # Arrange + with mock_aws(): + client = boto3.client("s3", region_name="us-east-1") + client.create_bucket(Bucket=_BUCKET) + client.put_object(Bucket=_BUCKET, Key=f"dev/{_LEGACY_KEY}", Body=b"stale") + client.put_object(Bucket=_BUCKET, Key=f"dev/{_CURRENT_KEY}", Body=b"live") + mocker.patch.object(purge_module, "build_s3_client", return_value=client) + + # Act + result = _invoke("--s3-bucket", _BUCKET, "--s3-prefix", "dev", "--apply") + + # Assert + assert result.exit_code == 0 + assert f"Target: s3://{_BUCKET}/dev" in result.output + assert "Deleted: 1" in result.output + remaining = { + obj["Key"] + for obj in client.list_objects_v2(Bucket=_BUCKET).get("Contents", ()) + } + assert remaining == {f"dev/{_CURRENT_KEY}"} + + def test_purge_legacy_cache_should_render_an_unprefixed_s3_target(self, mocker): + """Test that an empty prefix leaves no trailing slash. + + Given: + An S3 bucket configured with no prefix. + When: + The command is invoked. + Then: + The target line should read the bare bucket URL, so the + operator sees exactly the scope that will be swept. + """ + # Arrange + mocker.patch.object(purge_module, "build_s3_client", return_value=object()) + mocker.patch.object(purge_module, "purge_s3", return_value=PurgeReport()) + + # Act + result = _invoke("--s3-bucket", _BUCKET) + + # Assert + assert result.exit_code == 0 + assert f"Target: s3://{_BUCKET}\n" in result.output + + def test_purge_legacy_cache_should_resolve_every_option_from_the_environment( + self, monkeypatch, mocker + ): + """Test that the documented environment bindings are wired. + + Given: + WORKFLOW_S3_BUCKET, WORKFLOW_S3_PREFIX, AWS_ENDPOINT_URL, and + AWS_REGION set, with no flags passed. + When: + The command is invoked with --apply. + Then: + The client factory should receive the endpoint and region and + the sweep should receive the bucket and prefix — a dropped + endpoint would silently point a LocalStack sweep at real AWS. + """ + # Arrange + monkeypatch.setenv("WORKFLOW_S3_BUCKET", "env-bucket") + monkeypatch.setenv("WORKFLOW_S3_PREFIX", "staging") + monkeypatch.setenv("AWS_ENDPOINT_URL", "http://localstack:4566") + monkeypatch.setenv("AWS_REGION", "us-west-2") + factory = mocker.patch.object( + purge_module, "build_s3_client", return_value=object() + ) + sweep = mocker.patch.object( + purge_module, "purge_s3", return_value=PurgeReport() + ) + + # Act + result = _invoke("--apply") + + # Assert + assert result.exit_code == 0 + factory.assert_called_once_with( + endpoint_url="http://localstack:4566", region_name="us-west-2" + ) + assert sweep.call_args.args[1] == "env-bucket" + assert sweep.call_args.kwargs["prefix"] == "staging" + assert sweep.call_args.kwargs["apply"] is True + + def test_purge_legacy_cache_should_render_the_reclaimable_size(self, mocker): + """Test that the report sizes the sweep for an operator. + + Given: + A sweep reporting a multi-gibibyte byte total. + When: + The command is invoked as a dry run. + Then: + It should print the thousands-separated byte count alongside + a two-decimal GiB figure, so the operator can judge the + reclaim before committing to it. + """ + # Arrange + mocker.patch.object(purge_module, "build_s3_client", return_value=object()) + mocker.patch.object( + purge_module, + "purge_s3", + return_value=PurgeReport( + scanned=9, matched=4, deleted=0, bytes_matched=1_234_567_890 + ), + ) + + # Act + result = _invoke("--s3-bucket", _BUCKET) + + # Assert + assert "Scanned: 9" in result.output + assert "Legacy entries: 4 (1,234,567,890 bytes, 1.15 GiB)" in result.output diff --git a/tests/test_workflows/test_purge.py b/tests/test_workflows/test_purge.py new file mode 100644 index 0000000..8a9de11 --- /dev/null +++ b/tests/test_workflows/test_purge.py @@ -0,0 +1,985 @@ +"""Tests for the legacy cache-key purge sweep.""" + +from __future__ import annotations + +from pathlib import Path + +import boto3 +import pytest +from hypothesis import given, settings +from hypothesis import strategies as st +from moto import mock_aws + +from cfdb.workflows.cache import LocalFsCache, S3Cache +from cfdb.workflows.keys import cache_key +from cfdb.workflows.models import ArtifactKind +from cfdb.workflows.purge import ( + PurgeReport, + build_s3_client, + purge_local, + purge_s3, +) +from tests.test_workflows import FIXTURE_MD5 + +_BUCKET = "cfdb-test-purge" + +#: A key of the retired four-segment shape, as the pipeline minted before +#: the processor-identity segment existed. +_LEGACY_KEY = f"encode/ENCFF732YBO/index/{FIXTURE_MD5}-v2" + +#: The same artifact under the current scheme — must survive every sweep. +_CURRENT_KEY = cache_key( + dcc="encode", + local_id="ENCFF732YBO", + artifact_kind=ArtifactKind.INDEX, + md5=FIXTURE_MD5, + processor_id="tabix-interval", + processor_version=2, +) + + +@pytest.fixture() +def s3_client(): + """Return a moto-backed boto3 S3 client with one created bucket.""" + with mock_aws(): + client = boto3.client("s3", region_name="us-east-1") + client.create_bucket(Bucket=_BUCKET) + yield client + + +def _keys_in(client, bucket: str) -> set[str]: + """Return every object key currently in ``bucket``, across all pages.""" + keys: set[str] = set() + for page in client.get_paginator("list_objects_v2").paginate(Bucket=bucket): + keys.update(obj["Key"] for obj in page.get("Contents", ())) + return keys + + +def _seed_local(root: Path, key: str, payload: bytes = b"artifact") -> Path: + """Write ``payload`` into ``root`` at the cache-key path and return it.""" + path = root / key + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(payload) + return path + + +def _legacy_key(local_id: str) -> str: + """Return a retired-scheme key for ``local_id``.""" + return f"encode/{local_id}/index/{FIXTURE_MD5}-v2" + + +class _StubPage(dict): + """One ``list_objects_v2`` page, shaped as botocore returns it.""" + + +class _StubClient: + """A minimal S3 client double for the delete-response paths. + + ``purge_s3`` takes its client as a public parameter, so driving it + with a double stays on the public surface. A double is required + rather than moto because moto only populates ``Errors`` for versioned + deletes, which the sweep never issues — the failure mode this covers + is therefore unreachable through the real mock. + """ + + def __init__(self, keys, delete_responses): + self._keys = list(keys) + self._delete_responses = list(delete_responses) + self.delete_calls: list[list[str]] = [] + + def get_paginator(self, operation_name): + assert operation_name == "list_objects_v2" + return self + + def paginate(self, **kwargs): + contents = [{"Key": key, "Size": 4} for key in self._keys] + yield _StubPage(Contents=contents) + + def delete_objects(self, *, Bucket, Delete): + requested = [entry["Key"] for entry in Delete["Objects"]] + self.delete_calls.append(requested) + return self._delete_responses.pop(0) + + +class TestPurgeReport: + def test_purge_report_should_default_every_counter_to_zero(self): + """Test that a fresh report describes an examined-nothing sweep. + + Given: + No arguments. + When: + A PurgeReport is constructed. + Then: + All four counters should be zero — this is the value every + early-return path hands back, so it must read as "nothing + found" rather than as unset. + """ + # Act + report = PurgeReport() + + # Assert + assert ( + report.scanned, + report.matched, + report.deleted, + report.bytes_matched, + ) == (0, 0, 0, 0) + + +class TestBuildS3Client: + def test_build_s3_client_should_honor_the_supplied_endpoint_override(self): + """Test that the sweep's client targets the endpoint it is given. + + Given: + An explicit endpoint_url, as a LocalStack-backed environment + supplies through AWS_ENDPOINT_URL. + When: + build_s3_client is called with it. + Then: + It should return an s3 client bound to that endpoint, so a + dev sweep cannot be redirected at real AWS by the default + resolver chain. + """ + # Act & assert + with mock_aws(): + client = build_s3_client( + endpoint_url="http://localstack:4566", region_name="us-east-1" + ) + + assert client.meta.service_model.service_name == "s3" + assert client.meta.endpoint_url == "http://localstack:4566" + + def test_build_s3_client_should_resolve_a_default_endpoint_from_the_region(self): + """Test that a production sweep needs no endpoint configuration. + + Given: + A region but no endpoint override. + When: + build_s3_client is called. + Then: + It should return an s3 client pointed at AWS, so the common + production invocation works with nothing but a region. + """ + # Act & assert + with mock_aws(): + client = build_s3_client(region_name="us-east-1") + + assert client.meta.service_model.service_name == "s3" + assert "amazonaws.com" in client.meta.endpoint_url + + +class TestPurgeS3: + def test_purge_s3_should_delete_legacy_keys_and_keep_current_ones( + self, s3_client + ): + """Test that an applied sweep removes only retired-scheme objects. + + Given: + A bucket holding one legacy-scheme object and one current + key derived by cache_key. + When: + purge_s3 runs with apply=True. + Then: + It should delete the legacy object and leave the current one, + reporting one match and one deletion. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=_LEGACY_KEY, Body=b"stale") + s3_client.put_object(Bucket=_BUCKET, Key=_CURRENT_KEY, Body=b"live") + + # Act + report = purge_s3(s3_client, _BUCKET, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (2, 1, 1) + assert _keys_in(s3_client, _BUCKET) == {_CURRENT_KEY} + + def test_purge_s3_should_delete_nothing_when_not_applied(self, s3_client): + """Test that the default dry run reports without deleting. + + Given: + A bucket holding one legacy-scheme object. + When: + purge_s3 runs without apply. + Then: + It should report the match and its size but leave the object + in place, so an operator can size the sweep before committing. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=_LEGACY_KEY, Body=b"stale") + + # Act + report = purge_s3(s3_client, _BUCKET) + + # Assert + assert (report.matched, report.deleted, report.bytes_matched) == (1, 0, 5) + assert _keys_in(s3_client, _BUCKET) == {_LEGACY_KEY} + + def test_purge_s3_should_strip_the_configured_prefix_before_matching( + self, s3_client + ): + """Test that a bucket prefix is not counted as a key segment. + + Given: + A bucket whose cache lives under a ``dev/`` prefix, holding a + legacy-scheme entry and a current one. + When: + purge_s3 runs with that prefix and apply=True. + Then: + It should delete the prefixed legacy entry — the prefix is + the backend's namespacing, not part of the cache key, so + counting it would make every key look five-segment and match + nothing. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=f"dev/{_LEGACY_KEY}", Body=b"stale") + s3_client.put_object(Bucket=_BUCKET, Key=f"dev/{_CURRENT_KEY}", Body=b"live") + + # Act + report = purge_s3(s3_client, _BUCKET, prefix="dev", apply=True) + + # Assert + assert report.deleted == 1 + assert _keys_in(s3_client, _BUCKET) == {f"dev/{_CURRENT_KEY}"} + + def test_purge_s3_should_ignore_objects_outside_the_configured_prefix( + self, s3_client + ): + """Test that a sweep scoped to one prefix leaves its neighbours alone. + + Given: + Legacy-scheme entries under two environment prefixes. + When: + purge_s3 runs against only one of them with apply=True. + Then: + It should leave the other environment's entry untouched, so a + shared bucket can be purged one environment at a time. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=f"dev/{_LEGACY_KEY}", Body=b"stale") + s3_client.put_object(Bucket=_BUCKET, Key=f"prod/{_LEGACY_KEY}", Body=b"stale") + + # Act + report = purge_s3(s3_client, _BUCKET, prefix="dev", apply=True) + + # Assert + assert (report.scanned, report.deleted) == (1, 1) + assert _keys_in(s3_client, _BUCKET) == {f"prod/{_LEGACY_KEY}"} + + def test_purge_s3_should_keep_a_live_key_when_the_prefix_is_over_specified( + self, s3_client + ): + """Test that a mistyped prefix cannot delete the live cache. + + Given: + A bucket holding a current-scheme artifact under a "dev" + prefix, swept with a prefix carrying one segment too many — + the shape a typo in WORKFLOW_S3_PREFIX produces. + When: + purge_s3 runs with apply=True. + Then: + It should delete nothing and leave the artifact in place. + Over-stripping leaves the processor identity in the + artifact-kind slot, which the legacy predicate rejects; + without that check this sweep would empty a live cache and + the deletion is irreversible. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=f"dev/{_CURRENT_KEY}", Body=b"live") + + # Act + report = purge_s3(s3_client, _BUCKET, prefix="dev/encode", apply=True) + + # Assert + assert (report.matched, report.deleted) == (0, 0) + assert _keys_in(s3_client, _BUCKET) == {f"dev/{_CURRENT_KEY}"} + + def test_purge_s3_should_return_an_empty_report_for_an_empty_bucket( + self, s3_client + ): + """Test that a cold or already-swept bucket is a clean no-op. + + Given: + An empty bucket. + When: + purge_s3 runs with apply=True. + Then: + It should return a zeroed report without raising, so the + sweep is safe to run against an environment that has none. + """ + # Act + report = purge_s3(s3_client, _BUCKET, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (0, 0, 0) + + def test_purge_s3_should_leave_a_bucket_of_only_current_keys_untouched( + self, s3_client + ): + """Test that a fully-migrated cache survives the sweep. + + Given: + A bucket holding only current-scheme keys. + When: + purge_s3 runs with apply=True. + Then: + It should scan them, match none, and leave every object in + place — the steady state after one successful migration. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=_CURRENT_KEY, Body=b"live") + + # Act + report = purge_s3(s3_client, _BUCKET, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (1, 0, 0) + assert _keys_in(s3_client, _BUCKET) == {_CURRENT_KEY} + + def test_purge_s3_should_ignore_objects_that_are_not_cache_entries(self, s3_client): + """Test that the sweep never deletes objects it does not own. + + Given: + A bucket mixing a legacy entry with unrelated objects that + share it — a top-level README and a nested log file. + When: + purge_s3 runs with apply=True. + Then: + It should delete only the legacy entry while counting every + object as scanned, so a bucket shared with another workload + keeps its data. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=_LEGACY_KEY, Body=b"stale") + s3_client.put_object(Bucket=_BUCKET, Key="README.md", Body=b"notes") + s3_client.put_object( + Bucket=_BUCKET, Key=f"logs/2024/01/{FIXTURE_MD5}-v2", Body=b"log" + ) + + # Act + report = purge_s3(s3_client, _BUCKET, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (3, 1, 1) + assert _keys_in(s3_client, _BUCKET) == { + "README.md", + f"logs/2024/01/{FIXTURE_MD5}-v2", + } + + @pytest.mark.parametrize("prefix", ["dev", "dev/", "/dev/"]) + def test_purge_s3_should_normalize_the_prefix_decoration(self, s3_client, prefix): + """Test that a prefix's slashes cannot change what is swept. + + Given: + A bucket whose cache lives under "dev/", swept with the + prefix written bare, trailing-slashed, or fully slashed. + When: + purge_s3 runs with apply=True for each form. + Then: + It should report the same match every time, matching the + normalization S3Cache applies to the same value. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=f"dev/{_LEGACY_KEY}", Body=b"stale") + + # Act + report = purge_s3(s3_client, _BUCKET, prefix=prefix, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (1, 1, 1) + + def test_purge_s3_should_not_sweep_a_neighbouring_environment(self, s3_client): + """Test that a textual prefix match cannot cross environments. + + Given: + Legacy entries under "dev/", "dev-staging/", and "devops/" — + prefixes of which one is a strict textual prefix of the + others. + When: + purge_s3 runs with prefix="dev" and apply=True. + Then: + It should touch only the "dev/" entry, so purging one + environment in a shared bucket cannot take its neighbours + with it. + """ + # Arrange + for env in ("dev", "dev-staging", "devops"): + s3_client.put_object(Bucket=_BUCKET, Key=f"{env}/{_LEGACY_KEY}", Body=b"x") + + # Act + report = purge_s3(s3_client, _BUCKET, prefix="dev", apply=True) + + # Assert + assert (report.scanned, report.deleted) == (1, 1) + assert _keys_in(s3_client, _BUCKET) == { + f"dev-staging/{_LEGACY_KEY}", + f"devops/{_LEGACY_KEY}", + } + + def test_purge_s3_should_report_the_same_totals_dry_and_applied(self, s3_client): + """Test that the dry run is an honest preview of the real sweep. + + Given: + A bucket holding legacy entries of known differing sizes + alongside a current entry. + When: + purge_s3 runs first as a dry run and then with apply=True. + Then: + Both runs should report the same match count and byte total, + with only the applied run deleting — so an operator can size + the sweep before committing to it. + """ + # Arrange + s3_client.put_object(Bucket=_BUCKET, Key=_legacy_key("A"), Body=b"1234") + s3_client.put_object(Bucket=_BUCKET, Key=_legacy_key("B"), Body=b"123456") + s3_client.put_object(Bucket=_BUCKET, Key=_CURRENT_KEY, Body=b"live") + + # Act + preview = purge_s3(s3_client, _BUCKET) + applied = purge_s3(s3_client, _BUCKET, apply=True) + + # Assert + assert (preview.matched, preview.bytes_matched, preview.deleted) == (2, 10, 0) + assert (applied.matched, applied.bytes_matched, applied.deleted) == (2, 10, 2) + assert _keys_in(s3_client, _BUCKET) == {_CURRENT_KEY} + + def test_purge_s3_should_delete_beyond_a_single_request_batch(self, s3_client): + """Test that a sweep larger than one delete request loses nothing. + + Given: + A bucket holding more legacy objects than one DeleteObjects + request accepts, spanning more than one listing page. + When: + purge_s3 runs with apply=True. + Then: + It should delete every one of them, so neither the pagination + boundary nor the batch flush strands part of a production + cache while reporting success. + """ + # Arrange + keys = {_legacy_key(f"ENCFF{index:05d}") for index in range(1001)} + for key in keys: + s3_client.put_object(Bucket=_BUCKET, Key=key, Body=b"x") + + # Act + report = purge_s3(s3_client, _BUCKET, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (1001, 1001, 1001) + assert _keys_in(s3_client, _BUCKET) == set() + + def test_purge_s3_should_not_delete_any_batch_on_a_dry_run(self, s3_client): + """Test that the mid-sweep flush is gated on apply. + + Given: + A bucket holding more legacy objects than one delete request + accepts. + When: + purge_s3 runs without apply. + Then: + It should match them all, delete none, and leave the bucket + intact — the batch flush inside the scan loop must respect + the dry run as much as the final flush does. + """ + # Arrange + keys = {_legacy_key(f"ENCFF{index:05d}") for index in range(1001)} + for key in keys: + s3_client.put_object(Bucket=_BUCKET, Key=key, Body=b"x") + + # Act + report = purge_s3(s3_client, _BUCKET) + + # Assert + assert (report.matched, report.deleted) == (1001, 0) + assert _keys_in(s3_client, _BUCKET) == keys + + def test_purge_s3_should_raise_when_a_delete_reports_errors(self): + """Test that a partial failure cannot pass as a completed sweep. + + Given: + A client whose DeleteObjects reports a per-key failure in the + response body, as a missing s3:DeleteObject grant produces. + When: + purge_s3 runs with apply=True. + Then: + It should raise RuntimeError naming the failing key. S3 + reports these failures without raising, so silence is the + default and an operator would otherwise conclude a cache was + purged when nothing was. + """ + # Arrange + client = _StubClient( + keys=[_LEGACY_KEY], + delete_responses=[ + {"Errors": [{"Key": _LEGACY_KEY, "Message": "AccessDenied"}]} + ], + ) + + # Act & assert + with pytest.raises(RuntimeError, match="AccessDenied"): + purge_s3(client, _BUCKET, apply=True) + + def test_purge_s3_should_raise_when_a_later_batch_reports_errors(self): + """Test that a late failure is not masked by an early success. + + Given: + A client whose second DeleteObjects call reports an error + while the first succeeded, over more keys than one batch. + When: + purge_s3 runs with apply=True. + Then: + It should raise RuntimeError, so a permission or throttling + failure part-way through a large sweep is surfaced rather + than averaged away by the batches that worked. + """ + # Arrange + keys = [_legacy_key(f"ENCFF{index:05d}") for index in range(1001)] + client = _StubClient( + keys=keys, + delete_responses=[ + {"Deleted": [{"Key": key} for key in keys[:1000]]}, + {"Errors": [{"Key": keys[1000], "Message": "SlowDown"}]}, + ], + ) + + # Act & assert + with pytest.raises(RuntimeError, match="SlowDown"): + purge_s3(client, _BUCKET, apply=True) + + def test_purge_s3_should_raise_when_s3_confirms_fewer_keys_than_matched(self): + """Test that a silent undercount is not reported as success. + + Given: + A client whose DeleteObjects confirms fewer keys than were + requested and reports no errors at all. + When: + purge_s3 runs with apply=True. + Then: + It should raise RuntimeError. A key that was already absent + still comes back confirmed, so a shortfall means something + was neither deleted nor complained about, and the report + would otherwise claim a clean sweep. + """ + # Arrange + keys = [_legacy_key("A"), _legacy_key("B")] + client = _StubClient( + keys=keys, delete_responses=[{"Deleted": [{"Key": keys[0]}]}] + ) + + # Act & assert + with pytest.raises(RuntimeError, match="not fully purged"): + purge_s3(client, _BUCKET, apply=True) + + def test_purge_s3_should_issue_no_delete_request_on_a_dry_run(self): + """Test that the dry run is side-effect-free at the client boundary. + + Given: + A recording client over a bucket holding legacy keys. + When: + purge_s3 runs without apply. + Then: + DeleteObjects should never be invoked — the dry run is proven + inert at the API call, not merely by the bucket looking + unchanged afterwards. + """ + # Arrange + client = _StubClient(keys=[_LEGACY_KEY], delete_responses=[]) + + # Act + report = purge_s3(client, _BUCKET) + + # Assert + assert report.matched == 1 + assert client.delete_calls == [] + + @pytest.mark.asyncio + async def test_purge_s3_should_agree_with_the_prefix_s3_cache_writes_under( + self, s3_client, tmp_path + ): + """Test that the sweep strips exactly what the backend prepends. + + Given: + An artifact written through S3Cache with a configured prefix, + under a key derived by the current cache_key. + When: + purge_s3 sweeps the same bucket with the same prefix. + Then: + It should delete nothing and the artifact should still be + readable through the cache — pinning that the two modules + agree about the prefix rather than merely looking similar. + """ + # Arrange + cache = S3Cache(bucket=_BUCKET, prefix="dev", client=s3_client) + source = tmp_path / "artifact.tbi" + source.write_bytes(b"payload") + await cache.put(_CURRENT_KEY, source) + + # Act + report = purge_s3(s3_client, _BUCKET, prefix="dev", apply=True) + + # Assert + assert (report.scanned, report.matched) == (1, 0) + assert await cache.head(_CURRENT_KEY) is not None + + +class TestPurgeLocal: + def test_purge_local_should_delete_legacy_entries_and_keep_current_ones( + self, tmp_path + ): + """Test that an applied sweep removes only retired-scheme entries. + + Given: + A local cache root holding one legacy-scheme file and one + current-scheme file. + When: + purge_local runs with apply=True. + Then: + It should unlink the legacy file and leave the current one. + """ + # Arrange + legacy = _seed_local(tmp_path, _LEGACY_KEY) + current = _seed_local(tmp_path, _CURRENT_KEY) + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (2, 1, 1) + assert not legacy.exists() + assert current.exists() + + def test_purge_local_should_delete_nothing_when_not_applied(self, tmp_path): + """Test that the default dry run reports without deleting. + + Given: + A local cache root holding one legacy-scheme file. + When: + purge_local runs without apply. + Then: + It should report the match and leave the file on disk. + """ + # Arrange + legacy = _seed_local(tmp_path, _LEGACY_KEY, b"stale") + + # Act + report = purge_local(tmp_path) + + # Assert + assert (report.matched, report.deleted, report.bytes_matched) == (1, 0, 5) + assert legacy.exists() + + def test_purge_local_should_prune_directories_left_empty(self, tmp_path): + """Test that the sweep does not leave the retired tree behind. + + Given: + A cache root whose only content is one legacy-scheme file. + When: + purge_local runs with apply=True. + Then: + It should remove the now-empty directories under the root, + leaving the root itself in place. + """ + # Arrange + _seed_local(tmp_path, _LEGACY_KEY) + + # Act + purge_local(tmp_path, apply=True) + + # Assert + assert tmp_path.is_dir() + assert list(tmp_path.iterdir()) == [] + + def test_purge_local_should_return_an_empty_report_when_root_absent( + self, tmp_path + ): + """Test that a missing cache root is not an error. + + Given: + A path where no cache root was ever created. + When: + purge_local is called on it. + Then: + It should return a zeroed report, so running the sweep on a + deployment that never wrote a local cache is a no-op. + """ + # Act + report = purge_local(tmp_path / "never-created") + + # Assert + assert (report.scanned, report.matched, report.deleted) == (0, 0, 0) + + def test_purge_local_should_return_an_empty_report_when_root_is_a_file( + self, tmp_path + ): + """Test that a mistyped root pointing at a file is inert. + + Given: + A path that exists but is a regular file rather than a + directory. + When: + purge_local is called on it. + Then: + It should return a zeroed report rather than raise, so a + mistyped --local-root does nothing instead of failing loudly + part-way through. + """ + # Arrange + not_a_root = tmp_path / "cache" + not_a_root.write_bytes(b"not a directory") + + # Act + report = purge_local(not_a_root) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (0, 0, 0) + assert not_a_root.exists() + + def test_purge_local_should_keep_directories_that_still_hold_a_current_entry( + self, tmp_path + ): + """Test that pruning stops at the first surviving artifact. + + Given: + A legacy entry and a current entry sharing their leading + dcc and local_id directories. + When: + purge_local runs with apply=True. + Then: + It should delete the legacy entry while leaving the current + one and every directory above it, so a sweep cannot orphan a + live artifact by pruning its parents out from under it. + """ + # Arrange + legacy = _seed_local(tmp_path, _LEGACY_KEY) + current = _seed_local(tmp_path, _CURRENT_KEY) + + # Act + purge_local(tmp_path, apply=True) + + # Assert + assert not legacy.exists() + assert current.exists() + assert current.parent.is_dir() + + def test_purge_local_should_keep_an_unrelated_empty_directory(self, tmp_path): + """Test that the sweep prunes only what its own deletions emptied. + + Given: + A cache root holding one legacy entry alongside an unrelated + directory that was already empty. + When: + purge_local runs with apply=True. + Then: + The unrelated directory should survive. The root is + operator-supplied through --local-root, so a directory the + sweep never touched is not the sweep's to reclaim. + """ + # Arrange + _seed_local(tmp_path, _LEGACY_KEY) + unrelated = tmp_path / "staging" + unrelated.mkdir() + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert report.deleted == 1 + assert unrelated.is_dir() + + def test_purge_local_should_not_prune_on_a_dry_run(self, tmp_path): + """Test that a dry run leaves the tree shape untouched. + + Given: + A cache root whose only content is one legacy entry. + When: + purge_local runs without apply. + Then: + Every directory should remain, so the preview mutates nothing + at all rather than merely leaving the files in place. + """ + # Arrange + legacy = _seed_local(tmp_path, _LEGACY_KEY) + + # Act + purge_local(tmp_path) + + # Assert + assert legacy.exists() + assert legacy.parent.is_dir() + + def test_purge_local_should_ignore_files_that_are_not_cache_entries(self, tmp_path): + """Test that the local sweep never deletes what it does not own. + + Given: + A cache root holding a legacy entry alongside a file at the + root and a non-cache file at legacy depth. + When: + purge_local runs with apply=True. + Then: + It should count every file as scanned but delete only the + legacy entry. + """ + # Arrange + legacy = _seed_local(tmp_path, _LEGACY_KEY) + notes = _seed_local(tmp_path, "encode/ENCFF732YBO/index/notes.txt") + readme = tmp_path / "README" + readme.write_bytes(b"cache root") + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert (report.scanned, report.matched, report.deleted) == (3, 1, 1) + assert not legacy.exists() + assert notes.exists() and readme.exists() + + def test_purge_local_should_not_delete_a_directory_named_like_a_legacy_leaf( + self, tmp_path + ): + """Test that only regular files are treated as cache entries. + + Given: + A directory whose path spells a complete legacy key, holding + a file of its own. + When: + purge_local runs with apply=True. + Then: + It should leave the directory and its contents alone, so a + path that merely looks like an entry is not removed. + """ + # Arrange + impostor = tmp_path / _LEGACY_KEY + impostor.mkdir(parents=True) + inner = impostor / "payload" + inner.write_bytes(b"inner") + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert (report.matched, report.deleted) == (0, 0) + assert inner.exists() + + def test_purge_local_should_match_a_zero_byte_legacy_entry(self, tmp_path): + """Test that an empty artifact is still reclaimed. + + Given: + A legacy entry of zero bytes. + When: + purge_local runs with apply=True. + Then: + It should match and delete it, reporting zero bytes freed — + a falsy size must not be mistaken for "no entry". + """ + # Arrange + legacy = _seed_local(tmp_path, _LEGACY_KEY, b"") + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert (report.matched, report.deleted, report.bytes_matched) == (1, 1, 0) + assert not legacy.exists() + + def test_purge_local_should_report_nothing_on_a_second_sweep(self, tmp_path): + """Test that the sweep is idempotent. + + Given: + A cache root already swept once with apply=True. + When: + purge_local runs with apply=True again. + Then: + It should report nothing matched and raise nothing — the + re-run recovery the module documents for a partial failure + depends on this. + """ + # Arrange + _seed_local(tmp_path, _LEGACY_KEY) + _seed_local(tmp_path, _CURRENT_KEY) + purge_local(tmp_path, apply=True) + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert (report.matched, report.deleted) == (0, 0) + + @pytest.mark.asyncio + async def test_purge_local_should_keep_an_entry_written_by_the_cache_backend( + self, tmp_path + ): + """Test that the sweep agrees with what LocalFsCache writes. + + Given: + An artifact written through LocalFsCache under a key derived + by the current cache_key. + When: + purge_local runs with apply=True. + Then: + It should delete nothing and the artifact should still be + readable through the cache, pinning that producer and sweep + share one notion of the current key shape. + """ + # Arrange + cache = LocalFsCache(tmp_path) + source = tmp_path / "source.tbi" + source.write_bytes(b"payload") + await cache.put(_CURRENT_KEY, source) + + # Act + report = purge_local(tmp_path, apply=True) + + # Assert + assert report.matched == 0 + assert await cache.head(_CURRENT_KEY) is not None + + @settings(max_examples=25, deadline=None) + @given( + local_id=st.text( + alphabet=st.characters(whitelist_categories=("L", "N")), + min_size=1, + max_size=16, + ), + md5=st.text(alphabet="abcdef0123456789", min_size=32, max_size=32), + artifact_kind=st.sampled_from(list(ArtifactKind)), + processor_id=st.sampled_from(["tabix-interval", "bam-index", "passthrough"]), + version=st.integers(min_value=0, max_value=99), + ) + def test_purge_local_should_delete_exactly_what_cache_key_no_longer_mints( + self, tmp_path_factory, local_id, md5, artifact_kind, processor_id, version + ): + """Test that the sweep and the deriver agree across the input space. + + Given: + Any file identity, seeded into a cache root twice — once + under the key cache_key derives today, and once under its + retired four-segment analogue. + When: + purge_local runs with apply=True. + Then: + The retired entry should be gone and the derived one should + survive, for every draw. This is the property that catches + the two modules drifting apart: whatever cache_key mints is + exactly what the sweep must not touch. + """ + # Arrange + root = tmp_path_factory.mktemp("cache") + current = cache_key( + dcc="encode", + local_id=local_id, + artifact_kind=artifact_kind, + md5=md5, + processor_id=processor_id, + processor_version=version, + ) + retired = f"encode/{local_id}/{artifact_kind.value}/{md5}-v{version}" + current_path = _seed_local(root, current) + retired_path = _seed_local(root, retired) + + # Act + report = purge_local(root, apply=True) + + # Assert + assert report.deleted == 1 + assert not retired_path.exists() + assert current_path.exists() From 587c5c11978651bf01313b9731a3ecdcbd74d0f0 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 09:12:19 -0400 Subject: [PATCH 06/14] test: Pin per-processor cache scoping across the router and executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The identity property was pinned only at key derivation, never where a wrong answer would actually be served. The router now has to prove that an artifact cached under another processor's identity is a miss on GET, on HEAD, and on the readiness probe, and that an artifact sitting at a retired-scheme key is likewise unreachable — the latter being what makes the sweep safe to run and what leaves the shipping deploy cold. Several cache-hit tests had become tautologies. They rebuilt the expected key with the same expression cache_key_for evaluates, so they would have kept passing if the identity segment were dropped entirely. They now seed through the processor's own derivation, which is the agreement the router probe and the processor put actually depend on. Two fixtures held four-segment key literals under a comment claiming they matched what cache_key produces in production. That stopped being true, and the executor stub's keys reach real JobRecord.artifact_cache_keys in integration runs, so both are now derived. The executor also asserts the persisted keys equal what the processor derives, closing the chain from producer through job record to router probe. The end-to-end suites asserted only that a cached path existed, which holds under any key scheme. They now check the shape a real processor driven through a real worker actually wrote. test_lifespan_registry covers a hole the duplicate-identity guard made consequential: the existing lifespan test replaces default_registry with a MagicMock, so no test had ever run register against a registry already holding PassthroughProcessor. It also pins that startup builds a fresh registry, the one assignment keeping a re-entered lifespan from turning the guard into a boot crash-loop. Remaining changes are call-site updates for the new cache_key signature. --- tests/integration/routines.py | 9 +- tests/integration/test_direct_processors.py | 22 +- tests/integration/test_processor_e2e.py | 22 ++ tests/test_api/test_lifespan_registry.py | 135 +++++++++++ tests/test_cache_stream.py | 251 ++++++++++++++++++-- tests/test_data.py | 2 + tests/test_index.py | 2 + tests/test_workflows/test_executor.py | 42 +++- 8 files changed, 445 insertions(+), 40 deletions(-) create mode 100644 tests/test_api/test_lifespan_registry.py diff --git a/tests/integration/routines.py b/tests/integration/routines.py index 5f152f4..ca3daad 100644 --- a/tests/integration/routines.py +++ b/tests/integration/routines.py @@ -78,9 +78,14 @@ def __init__( sleep_between_yields: float = 0.0, unpicklable_field: Any | None = None, ) -> None: + # Derived rather than written as literals so the keys this stub + # emits — and which reach real JobRecord.artifact_cache_keys in + # integration runs — carry the production key shape, including + # the processor-identity segment. Hard-coded four-segment keys + # would be retired-scheme strings the purge sweep claims. self.artifacts = artifacts or { - ArtifactKind.DATA.value: f"encode/x/data/{STUB_MD5}-v0", - ArtifactKind.INDEX.value: f"encode/x/index/{STUB_MD5}-v0", + kind.value: self.cache_key_for(stub_file_meta(), kind) + for kind in (ArtifactKind.DATA, ArtifactKind.INDEX) } self.raise_during_stage = raise_during_stage self.sleep_seconds = sleep_seconds diff --git a/tests/integration/test_direct_processors.py b/tests/integration/test_direct_processors.py index 9d166d2..7acb781 100644 --- a/tests/integration/test_direct_processors.py +++ b/tests/integration/test_direct_processors.py @@ -183,16 +183,14 @@ async def recording_run_shell(cmd: str) -> None: # processor's ``cache.head(data_key)`` short-circuits the # convert+sort pipeline. cache = LocalFsCache(cache_root) - from cfdb.workflows import keys as key_utils from cfdb.workflows.models import ArtifactKind - data_key = key_utils.cache_key( - dcc="encode", - local_id="int-warm-sam", - artifact_kind=ArtifactKind.DATA, - md5="098f6bcd4621d373cade4e832627b4f6", - processor_version=BamIndexProcessor.processor_version, - ) + # Seed through the processor's own derivation. Restating the + # formula here would make the test pass whatever key the + # processor actually probes, which is exactly the agreement the + # warm-cache short-circuit depends on. + processor = BamIndexProcessor() + data_key = processor.cache_key_for(file_meta, ArtifactKind.DATA) await cache.put(data_key, prebuilt_bam) # Act @@ -213,13 +211,7 @@ async def recording_run_shell(cmd: str) -> None: f"cache is warm; got invocations: {shell_invocations!r}" ) index_entry = await cache.head( - key_utils.cache_key( - dcc="encode", - local_id="int-warm-sam", - artifact_kind=ArtifactKind.INDEX, - md5="098f6bcd4621d373cade4e832627b4f6", - processor_version=BamIndexProcessor.processor_version, - ) + processor.cache_key_for(file_meta, ArtifactKind.INDEX) ) assert index_entry is not None and index_entry.size > 0 diff --git a/tests/integration/test_processor_e2e.py b/tests/integration/test_processor_e2e.py index 780eb23..d648e18 100644 --- a/tests/integration/test_processor_e2e.py +++ b/tests/integration/test_processor_e2e.py @@ -29,6 +29,7 @@ import pytest from allpairspy import AllPairs +from cfdb.workflows.keys import is_legacy_cache_key from cfdb.workflows.lock import get_job from cfdb.workflows.models import JobStatus @@ -46,6 +47,24 @@ pytestmark = pytest.mark.integration +def _assert_production_key_shape(record, executor) -> None: + """Assert every persisted artifact key carries a processor identity. + + The e2e assertions elsewhere in this file join a cached path from + ``artifact_cache_keys`` and check the bytes, which stays true under + any key scheme. This is the one place a real processor, driven + through a real worker, is made to prove it wrote under the current + five-segment shape — and that the sweep would not claim what it + just produced. + """ + processor = executor._registry.lookup_for(record.file_meta_snapshot) + for key in record.artifact_cache_keys.values(): + segments = key.split("/") + assert len(segments) == 5, key + assert segments[3] == processor.processor_id, key + assert is_legacy_cache_key(key) is False, key + + def _stage_for_tabix(cached_bgz: Path, cached_tbi: Path, stage_dir: Path) -> Path: """Copy a cached bgz + tbi pair into ``stage_dir`` with tabix-friendly names. @@ -167,6 +186,7 @@ async def _body(): assert final.status == JobStatus.COMPLETED assert final.stages_done == ["index"] assert "data" not in final.artifact_cache_keys + _assert_production_key_shape(final, integration_executor) cache_root = integration_executor._cache.root cached_bai = cache_root / final.artifact_cache_keys["index"] @@ -274,6 +294,7 @@ async def _body(): # Assert final = await get_job(install_jobs_index, record.job_id) assert final is not None and final.status == JobStatus.COMPLETED + _assert_production_key_shape(final, integration_executor) cache_root = integration_executor._cache.root cached_bgz = cache_root / final.artifact_cache_keys["data"] @@ -356,6 +377,7 @@ async def _body(): # Assert final = await get_job(install_jobs_index, record.job_id) assert final is not None and final.status == JobStatus.COMPLETED + _assert_production_key_shape(final, integration_executor) cache_root = integration_executor._cache.root cached_bgz = cache_root / final.artifact_cache_keys["data"] diff --git a/tests/test_api/test_lifespan_registry.py b/tests/test_api/test_lifespan_registry.py new file mode 100644 index 0000000..f4d0638 --- /dev/null +++ b/tests/test_api/test_lifespan_registry.py @@ -0,0 +1,135 @@ +"""Tests for the processor registry the lifespan wires at startup. + +``test_lifespan_workerpool`` replaces ``default_registry`` with a +``MagicMock``, so the ``register(BamIndexProcessor())`` and +``register(TabixIntervalProcessor())`` calls in ``cfdb.api.main`` land on +a mock and their real effect is never exercised. That mattered little +until issue #109 gave ``ProcessorRegistry.register`` a failure mode: +registering two processors that share a ``processor_id`` now raises. The +shipped wiring is the guard's only production caller, so these tests +drive the lifespan with ``default_registry`` left real. +""" + +from __future__ import annotations + +import contextlib + +import pytest + +from cfdb import api +from cfdb.api import main +from cfdb.api import profile as profile_mod +from cfdb.workflows.processors.bam import BamIndexProcessor +from cfdb.workflows.processors.passthrough import PassthroughProcessor +from cfdb.workflows.processors.tabix import TabixIntervalProcessor + + +@contextlib.asynccontextmanager +async def _stubbed_lifespan(mocker, tmp_path): + """Yield the app lifespan with everything but the registry stubbed.""" + from mongomock_motor import AsyncMongoMockClient + + profile = profile_mod.WorkflowProfile( + kind="local", + cache_root=tmp_path / "cache", + workdir_root=tmp_path / "jobs", + ) + + class _PoolStub: + def __init__(self, **kwargs): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *_exc): + return None + + @contextlib.asynccontextmanager + async def _fake_build_discovery(_profile): + yield object() + + executor = mocker.MagicMock() + executor.drain = mocker.AsyncMock(return_value=0) + + mocker.patch.object( + main, "create_mongodb_client", return_value=AsyncMongoMockClient() + ) + mocker.patch.object(main.WorkflowProfile, "from_env", return_value=profile) + mocker.patch.object( + main, "_build_cache", new=mocker.AsyncMock(return_value=mocker.MagicMock()) + ) + mocker.patch.object(main, "_build_provisioner", return_value=None) + mocker.patch.object(main, "_build_discovery", new=_fake_build_discovery) + mocker.patch.object(main, "build_worker_credentials", return_value="CREDS-SENTINEL") + mocker.patch.object(main, "WoolExecutor", return_value=executor) + mocker.patch.object(main.wool, "WorkerPool", _PoolStub) + + async with main.lifespan(main.app): + yield + + +@pytest.mark.asyncio +async def test_lifespan_should_wire_every_shipped_processor(mocker, tmp_path): + """Test that startup builds a registry resolving all shipped formats. + + Given: + The workflow subsystem enabled and ``default_registry`` left + unpatched, so the real registrations run. + When: + The app lifespan starts up. + Then: + ``api.processor_registry`` should resolve BAM, BED, and bigWig to + their processors — proving the three shipped identities are + distinct enough for the duplicate guard to admit them all, which + no other test exercises. + """ + # Act + async with _stubbed_lifespan(mocker, tmp_path): + registry = api.processor_registry + + # Assert + assert isinstance( + registry.lookup_for({"file_format": {"name": "BAM"}}), BamIndexProcessor + ) + assert isinstance( + registry.lookup_for({"file_format": {"name": "BED"}}), + TabixIntervalProcessor, + ) + assert isinstance( + registry.lookup_for({"file_format": {"name": "bigWig"}}), + PassthroughProcessor, + ) + + +@pytest.mark.asyncio +async def test_lifespan_should_build_a_fresh_registry_on_every_startup( + mocker, tmp_path +): + """Test that a second startup does not re-register onto the first. + + Given: + The same enabled lifespan, with ``default_registry`` unpatched. + When: + The lifespan is entered and exited twice in sequence, as a + reload or a worker restart does. + Then: + Neither run should raise and the second registry should still be + well-formed. Startup assigns a fresh registry immediately before + registering, and that one assignment is the only reason the + duplicate guard cannot turn a benign restart into a boot + crash-loop. + """ + # Arrange + async with _stubbed_lifespan(mocker, tmp_path): + first = api.processor_registry + + # Act + async with _stubbed_lifespan(mocker, tmp_path): + second = api.processor_registry + + # Assert + assert second is not first + assert isinstance( + second.lookup_for({"file_format": {"name": "BAM"}}), BamIndexProcessor + ) diff --git a/tests/test_cache_stream.py b/tests/test_cache_stream.py index e415829..d88fcd8 100644 --- a/tests/test_cache_stream.py +++ b/tests/test_cache_stream.py @@ -206,6 +206,32 @@ async def run(self, file_meta, workdir, cache_root): yield {"event": "complete", "artifacts": {}} +#: The key the retired scheme minted for ``_file_doc``'s identity. An +#: artifact sitting here must be unreachable through every serving path, +#: which is what makes the ``purge-legacy-cache`` sweep safe to run. +_LEGACY_CACHE_KEY = f"encode/ENCFF123/data/{FIXTURE_MD5}-v0" + + +def _foreign_processor_key(processor: Processor) -> str: + """Return the key a *different* processor derives for the same file. + + Identical in every component the two processors share — file, artifact + kind, md5, and processor version — differing only in the identity + segment. That is precisely the artifact the pre-#109 scheme would have + served as a cache hit. + """ + from cfdb.workflows import keys as key_utils + + return key_utils.cache_key( + dcc="encode", + local_id="ENCFF123", + artifact_kind=ArtifactKind.DATA, + md5=FIXTURE_MD5, + processor_id="some-other-processor", + processor_version=processor.processor_version, + ) + + def _file_doc(**overrides) -> dict[str, Any]: """Return a minimal file_doc accepted by extract_identity.""" doc = { @@ -393,21 +419,17 @@ async def test_should_stream_on_cache_hit_for_get(self, mocker, tmp_path): the cached bytes. """ # Arrange - from cfdb.workflows import keys as key_utils - processor = _StubProcessor() registry = ProcessorRegistry() registry.register(processor) cache = LocalFsCache(tmp_path / "cache") src = tmp_path / "src" src.write_bytes(b"cached-data") - key = key_utils.cache_key( - dcc="encode", - local_id="ENCFF123", - artifact_kind=ArtifactKind.DATA, - md5=FIXTURE_MD5, - processor_version=processor.processor_version, - ) + # Seed through the processor's own derivation rather than + # restating the formula: this test is about the router probing + # the key the processor writes under, so re-deriving it here + # would pass even if the two stopped agreeing. + key = processor.cache_key_for(_file_doc(), ArtifactKind.DATA) await cache.put(key, src) mocker.patch.object(api, "processor_registry", registry) mocker.patch.object(api, "cache", cache) @@ -459,6 +481,135 @@ async def test_should_raise_404_on_head_cache_miss(self, mocker, tmp_path): assert "head-detail-marker" in (exc_info.value.detail or "") assert executor.calls == [] + @pytest.mark.asyncio + async def test_should_dispatch_when_only_another_processors_artifact_is_cached( + self, mocker, tmp_path + ): + """Test that one processor never serves another's artifact. + + Given: + A cache holding an artifact for the same file, artifact kind, + md5, and processor version, but written under a *different* + processor's identity — and a GET. + When: + The helper is awaited. + Then: + It should treat the cache as a miss and dispatch a fresh + workflow. This is the defect the issue exists to close, made + observable where it would actually serve a wrong answer + rather than only at key derivation. + """ + # Arrange + processor = _StubProcessor() + registry = ProcessorRegistry() + registry.register(processor) + cache = LocalFsCache(tmp_path / "cache") + src = tmp_path / "src" + src.write_bytes(b"other-processors-artifact") + await cache.put(_foreign_processor_key(processor), src) + executor = _RecordingExecutor(result=(_make_record(), True)) + mocker.patch.object(api, "processor_registry", registry) + mocker.patch.object(api, "cache", cache) + mocker.patch.object(api, "executor", executor) + + # Act + resp = await serve_workflow_artifact_or_dispatch( + _file_doc(), + ArtifactKind.DATA, + _Request(), + None, + head_404_detail="missing", + ) + + # Assert + assert isinstance(resp, JSONResponse) + assert resp.status_code == 202 + assert len(executor.calls) == 1 + + @pytest.mark.asyncio + async def test_should_raise_404_on_head_when_only_another_processors_artifact_is_cached( + self, mocker, tmp_path + ): + """Test that the cross-processor miss holds on the probe path too. + + Given: + The same cache seeded under a different processor's identity, + and a HEAD request. + When: + The helper is awaited. + Then: + It should raise ``HTTPException(404)`` and dispatch nothing, + so the side-effect-free path agrees that another processor's + artifact is not this one's. + """ + # Arrange + processor = _StubProcessor() + registry = ProcessorRegistry() + registry.register(processor) + cache = LocalFsCache(tmp_path / "cache") + src = tmp_path / "src" + src.write_bytes(b"other-processors-artifact") + await cache.put(_foreign_processor_key(processor), src) + executor = _RecordingExecutor() + mocker.patch.object(api, "processor_registry", registry) + mocker.patch.object(api, "cache", cache) + mocker.patch.object(api, "executor", executor) + + # Act & assert + with pytest.raises(HTTPException) as exc_info: + await serve_workflow_artifact_or_dispatch( + _file_doc(), + ArtifactKind.DATA, + _Request("HEAD"), + None, + head_404_detail="missing", + ) + assert exc_info.value.status_code == 404 + assert executor.calls == [] + + @pytest.mark.asyncio + async def test_should_dispatch_when_only_a_legacy_key_artifact_is_cached( + self, mocker, tmp_path + ): + """Test that a retired-scheme artifact is unreachable. + + Given: + A cache holding an artifact for this file under the retired + four-segment key, and a GET. + When: + The helper is awaited. + Then: + It should dispatch a fresh workflow rather than serve it. + This is what makes the purge safe: nothing reads a legacy + key, so deleting one cannot take a servable artifact with it, + and the deploy shipping this change starts fully cold. + """ + # Arrange + registry = ProcessorRegistry() + registry.register(_StubProcessor()) + cache = LocalFsCache(tmp_path / "cache") + src = tmp_path / "src" + src.write_bytes(b"stale-artifact") + await cache.put(_LEGACY_CACHE_KEY, src) + executor = _RecordingExecutor(result=(_make_record(), True)) + mocker.patch.object(api, "processor_registry", registry) + mocker.patch.object(api, "cache", cache) + mocker.patch.object(api, "executor", executor) + + # Act + resp = await serve_workflow_artifact_or_dispatch( + _file_doc(), + ArtifactKind.DATA, + _Request(), + None, + head_404_detail="missing", + ) + + # Assert + assert isinstance(resp, JSONResponse) + assert resp.status_code == 202 + assert len(executor.calls) == 1 + @pytest.mark.asyncio async def test_should_raise_503_when_executor_is_draining( self, mocker, tmp_path @@ -745,21 +896,15 @@ async def test_probe_workflow_readiness_should_return_true_on_cache_hit( called. """ # Arrange - from cfdb.workflows import keys as key_utils - processor = _StubProcessor() registry = ProcessorRegistry() registry.register(processor) cache = LocalFsCache(tmp_path / "cache") src = tmp_path / "src" src.write_bytes(b"cached-data") - key = key_utils.cache_key( - dcc="encode", - local_id="ENCFF123", - artifact_kind=ArtifactKind.DATA, - md5=FIXTURE_MD5, - processor_version=processor.processor_version, - ) + # Seeded through the processor's own derivation — see the + # equivalent note on the dispatch-path cache-hit test. + key = processor.cache_key_for(_file_doc(), ArtifactKind.DATA) await cache.put(key, src) executor = _RecordingExecutor() mocker.patch.object(api, "processor_registry", registry) @@ -773,6 +918,76 @@ async def test_probe_workflow_readiness_should_return_true_on_cache_hit( assert result is True assert executor.calls == [] + @pytest.mark.asyncio + async def test_probe_workflow_readiness_should_return_false_for_a_foreign_key( + self, mocker, tmp_path + ): + """Test that the probe agrees another processor's artifact is a miss. + + Given: + A cache seeded only under a different processor's identity + for this file and artifact kind. + When: + The probe is awaited. + Then: + It should return False, matching what a GET would actually + do — the readiness probe and the dispatch path must not + disagree across the identity seam. + """ + # Arrange + processor = _StubProcessor() + registry = ProcessorRegistry() + registry.register(processor) + cache = LocalFsCache(tmp_path / "cache") + src = tmp_path / "src" + src.write_bytes(b"other-processors-artifact") + await cache.put(_foreign_processor_key(processor), src) + executor = _RecordingExecutor() + mocker.patch.object(api, "processor_registry", registry) + mocker.patch.object(api, "cache", cache) + mocker.patch.object(api, "executor", executor) + + # Act + result = await probe_workflow_readiness(_file_doc(), ArtifactKind.DATA) + + # Assert + assert result is False + assert executor.calls == [] + + @pytest.mark.asyncio + async def test_probe_workflow_readiness_should_return_false_for_a_legacy_key( + self, mocker, tmp_path + ): + """Test that the probe reports a retired-scheme artifact as absent. + + Given: + A cache seeded only under the retired four-segment key for + this file. + When: + The probe is awaited. + Then: + It should return False, so ``/status`` tells the truth about + a cache the migration has made cold. + """ + # Arrange + registry = ProcessorRegistry() + registry.register(_StubProcessor()) + cache = LocalFsCache(tmp_path / "cache") + src = tmp_path / "src" + src.write_bytes(b"stale-artifact") + await cache.put(_LEGACY_CACHE_KEY, src) + executor = _RecordingExecutor() + mocker.patch.object(api, "processor_registry", registry) + mocker.patch.object(api, "cache", cache) + mocker.patch.object(api, "executor", executor) + + # Act + result = await probe_workflow_readiness(_file_doc(), ArtifactKind.DATA) + + # Assert + assert result is False + assert executor.calls == [] + @pytest.mark.asyncio async def test_probe_workflow_readiness_should_return_false_on_cache_miss( self, mocker, tmp_path diff --git a/tests/test_data.py b/tests/test_data.py index d27bb08..d3b25d7 100644 --- a/tests/test_data.py +++ b/tests/test_data.py @@ -443,6 +443,7 @@ async def test_stream_file_should_serve_cached_data_artifact_for_sam( local_id="4DNFISAM01", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) await cache.put(data_key, src) @@ -810,6 +811,7 @@ async def test_stream_file_status_should_report_ready_on_cache_hit_for_sam( local_id="4DNFISAM01", artifact_kind=ArtifactKind.DATA, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) await cache.put(data_key, src) diff --git a/tests/test_index.py b/tests/test_index.py index 12a10c5..8f5ea80 100644 --- a/tests/test_index.py +++ b/tests/test_index.py @@ -765,6 +765,7 @@ async def test_stream_index_file_should_serve_cached_index_without_dispatch( local_id="4DNFIBAM01", artifact_kind=ArtifactKind.INDEX, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) await cache.put(index_key, src) @@ -1335,6 +1336,7 @@ async def test_stream_index_file_status_should_report_ready_on_cache_hit( local_id="4DNFIBAM01", artifact_kind=ArtifactKind.INDEX, md5=FIXTURE_MD5, + processor_id=processor.processor_id, processor_version=processor.processor_version, ) await cache.put(index_key, src) diff --git a/tests/test_workflows/test_executor.py b/tests/test_workflows/test_executor.py index b3d0071..d5033d9 100644 --- a/tests/test_workflows/test_executor.py +++ b/tests/test_workflows/test_executor.py @@ -40,11 +40,13 @@ from cfdb.workflows.provisioner import EcsProvisioner, RetryableProvisionerError from tests.test_workflows import FIXTURE_MD5 -#: Canonical artifact keys the stubs emit. Built from FIXTURE_MD5 so the -#: shape matches what ``cache_key`` would produce in production for the -#: ``_file_meta`` identity below. -_DATA_KEY = f"encode/ENCFF123/data/{FIXTURE_MD5}-v0" -_INDEX_KEY = f"encode/ENCFF123/index/{FIXTURE_MD5}-v0" +#: Identity the stub processors are dispatched for. Kept beside the key +#: constants below so the two cannot drift. +_STUB_IDENTITY = { + "dcc": {"dcc_abbreviation": "ENCODE"}, + "local_id": "ENCFF123", + "md5": FIXTURE_MD5, +} class _StubProcessor(Processor): @@ -80,6 +82,27 @@ async def run( yield Complete(artifacts=dict(self.artifacts)) +def _stub_key(artifact_kind: ArtifactKind) -> str: + """Return the key ``_StubProcessor`` writes ``artifact_kind`` under.""" + return key_utils.cache_key( + dcc=_STUB_IDENTITY["dcc"]["dcc_abbreviation"], + local_id=_STUB_IDENTITY["local_id"], + artifact_kind=artifact_kind, + md5=_STUB_IDENTITY["md5"], + processor_id=_StubProcessor.processor_id, + processor_version=_StubProcessor.processor_version, + ) + + +#: Canonical artifact keys the stubs emit. Derived rather than written as +#: literals so they carry the production key shape — including the +#: processor-identity segment. The previous literals were retired-scheme +#: four-segment strings, which the purge sweep would have claimed. +#: Defined after the class because they read its class attributes. +_DATA_KEY = _stub_key(ArtifactKind.DATA) +_INDEX_KEY = _stub_key(ArtifactKind.INDEX) + + class _FailingProcessor(Processor): """Test double whose ``run`` raises mid-stream to exercise the error path.""" @@ -357,6 +380,15 @@ async def test_ensure_workflow_should_claim_and_run_fresh_job( assert final.status == JobStatus.COMPLETED assert final.artifact_cache_keys["data"] == _DATA_KEY assert final.artifact_cache_keys["index"] == _INDEX_KEY + # The persisted keys must be the ones the processor itself + # derives, so the producer, the job record, and the router's + # later probe agree by construction rather than coincidence. + assert final.artifact_cache_keys["data"] == processor.cache_key_for( + _file_meta(), ArtifactKind.DATA + ) + assert final.artifact_cache_keys["index"] == processor.cache_key_for( + _file_meta(), ArtifactKind.INDEX + ) @pytest.mark.asyncio async def test_ensure_workflow_should_attach_to_existing_job( From cf3128c0f95778afad2111ceb8d2759f8ff34092 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:43:24 -0400 Subject: [PATCH 07/14] fixup! feat: Fold a processor identity into the workflow cache key --- src/cfdb/workflows/keys.py | 89 ++++++++++++++++++++++---- src/cfdb/workflows/processors/base.py | 45 ++++++++++--- src/cfdb/workflows/processors/tabix.py | 3 +- 3 files changed, 117 insertions(+), 20 deletions(-) diff --git a/src/cfdb/workflows/keys.py b/src/cfdb/workflows/keys.py index dd85f2e..6c9a240 100644 --- a/src/cfdb/workflows/keys.py +++ b/src/cfdb/workflows/keys.py @@ -32,6 +32,14 @@ #: the content address followed by the producing processor's version. _CACHE_LEAF_RE = re.compile(r"^[a-f0-9]{32}-v\d+$") +#: Alphabet a processor identity may draw from. An allowlist rather than a +#: denylist because the value becomes both an S3 object-key segment and a +#: filesystem directory name: a control character, a zero-width space, or a +#: Unicode solidus lookalike is invisible in review but addresses a +#: different cache. Every shipped identity and every legal Python class +#: name (the default identity) is a subset of this. +_PROCESSOR_ID_RE = re.compile(r"^[A-Za-z0-9._-]+$") + #: Segment count of the retired (pre-#109) cache key #: ``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}``. The #: current scheme carries a processor-identity segment and so is one @@ -44,8 +52,20 @@ #: The legal artifact-kind segment values, as strings. Both key schemes #: place one here, so it is the segment that tells a cache key apart from #: an unrelated object sharing the bucket. +#: +#: This set is coupled to the processor-identity namespace: +#: :func:`normalize_processor_id` rejects an identity equal to one of these +#: values, and validates at class-definition time. Adding a member to +#: :class:`~cfdb.workflows.models.ArtifactKind` therefore retroactively +#: invalidates any shipped ``processor_id`` that matches it, at import — +#: which would crash-loop the API at boot. ``TestShippedProcessorIdentities`` +#: pins the disjointness so that lands in CI instead. _ARTIFACT_KIND_VALUES = frozenset(kind.value for kind in ArtifactKind) +#: Segments that traverse the path without containing a separator. Rejected +#: wherever a value becomes a key segment. +_TRAVERSAL_SEGMENTS = (".", "..") + def normalize_dcc(dcc: str) -> str: """Canonical DCC form used by both ``workflow_key`` and ``cache_key``. @@ -53,10 +73,18 @@ def normalize_dcc(dcc: str) -> str: Stripping whitespace and lower-casing is shared with ``extract_identity`` so a record's stored ``dcc`` field matches the substring embedded in its ``workflow_key``. + + Rejects a path traversal for the same reason + :func:`normalize_processor_id` does — the value becomes a key segment, + and ``"."`` is silently collapsed by the local backend's path + resolution, landing one logical key at two different depths on the two + cache backends. """ cleaned = dcc.strip().lower() if not cleaned: raise ValueError("dcc is required for workflow/cache key derivation") + if cleaned in _TRAVERSAL_SEGMENTS: + raise ValueError(f"dcc must not be a path traversal: {dcc!r}") return cleaned @@ -81,12 +109,22 @@ def normalize_local_id(local_id: str) -> str: can't smuggle into cache paths or shell pipelines as a directory segment. Case is preserved because upstream DCCs treat local_ids as opaque accessions (case-sensitive). + + ``"."`` and ``".."`` are rejected too. This value is the one component + of a cache key that comes from third-party DCC metadata rather than + from our own source, so it is the one that most needs the guard: + ``cache.py``'s ``_validate_cache_key`` refuses ``".."`` at ``put`` time + but accepts ``"."``, which the local backend's path resolution then + silently collapses — landing one logical key at five segments on S3 and + four on disk. """ cleaned = local_id.strip() if local_id else "" if not cleaned: raise ValueError("local_id is required for workflow/cache key derivation") if "/" in cleaned or "\\" in cleaned or "\x00" in cleaned: raise ValueError(f"local_id contains forbidden chars: {local_id!r}") + if cleaned in _TRAVERSAL_SEGMENTS: + raise ValueError(f"local_id must not be a path traversal: {local_id!r}") return cleaned @@ -94,34 +132,54 @@ def normalize_processor_id(processor_id: str) -> str: """Canonical processor-identity form embedded in ``cache_key``. Strips whitespace and preserves case. Rejects an empty (or - whitespace-only) value and any path-separator or null-byte character, - for the same reason ``normalize_local_id`` does: the value becomes a - path segment in the cache key, and a stray ``/`` would silently - restructure the key rather than fail. + whitespace-only) value, and constrains what remains to + ``[A-Za-z0-9._-]+`` — an allowlist rather than a denylist of the + separators, because the value becomes both an S3 object-key segment and + a filesystem directory name. A denylist catching ``/``, ``\\`` and + ``\\x00`` still admits a newline, a zero-width space, or a fullwidth + solidus, each of which is invisible in review while addressing a + different cache entry. Case is preserved because the default identity is a processor's class name (see ``Processor.__init_subclass__``), and folding case would - merge ``BedProcessor`` with a hypothetical ``BEDProcessor``. + merge ``BedProcessor`` with a hypothetical ``BEDProcessor``. Note that + preserving case here does **not** by itself keep two such identities + apart on disk: a case-insensitive filesystem (APFS by default) folds + the two directory names together. ``ProcessorRegistry.register`` is + where that collision is actually refused. Two further values are rejected, both because of what they would do to :func:`is_legacy_cache_key` rather than to the key itself: - ``"."`` and ``".."`` traverse a path segment without containing a - separator. ``cache.py``'s ``_validate_cache_key`` already refuses - them at ``put`` / ``head`` time, but that surfaces as a failure deep - inside a workflow; rejecting here fails at derivation instead. + separator, so the allowlist above admits them. ``cache.py``'s + ``_validate_cache_key`` already refuses them at ``put`` / ``head`` + time, but that surfaces as a failure deep inside a workflow; + rejecting here fails at derivation instead. - A value equal to an :class:`ArtifactKind` would let an over-specified purge prefix strip a live key down to something shaped exactly like a retired one — the processor id would land in the artifact-kind slot and satisfy that segment's check. See :func:`is_legacy_cache_key`. + + Raises ``ValueError`` for every rejection, including a non-``str`` + input: both request-path callers catch ``ValueError`` to fall through + to direct upstream streaming, so leaking an ``AttributeError`` from + ``.strip()`` would surface as a 500 rather than that fall-through. """ - cleaned = processor_id.strip() if processor_id else "" + if not isinstance(processor_id, str): + # ValueError, not TypeError (hence the noqa): both request-path + # callers catch ValueError to fall through to direct upstream + # streaming, so a TypeError here would surface as a 500 instead. + raise ValueError( # noqa: TRY004 + f"processor_id must be a str; got {type(processor_id).__name__}" + ) + cleaned = processor_id.strip() if not cleaned: raise ValueError("processor_id is required for cache key derivation") - if "/" in cleaned or "\\" in cleaned or "\x00" in cleaned: + if not _PROCESSOR_ID_RE.fullmatch(cleaned): raise ValueError(f"processor_id contains forbidden chars: {processor_id!r}") - if cleaned in (".", ".."): + if cleaned in _TRAVERSAL_SEGMENTS: raise ValueError(f"processor_id must not be a path traversal: {processor_id!r}") if cleaned in _ARTIFACT_KIND_VALUES: raise ValueError( @@ -159,12 +217,21 @@ def is_legacy_cache_key(key: str) -> bool: artifact-kind slot and fails; stripping two or more segments leaves too few to match at all. :func:`normalize_processor_id` forbids an identity equal to an artifact kind, closing the remaining overlap. + + A traversal segment is refused for the same reason: ``cache.py``'s + ``_validate_cache_key`` rejected ``".."`` at ``put`` time, so the + retired scheme provably never minted such a key — and ``purge_s3`` + deletes through ``client.delete_objects`` directly, bypassing that + validation. A shape the producer could not produce must not be claimed + by the predicate that authorizes deletion. """ segments = key.split("/") if len(segments) != _LEGACY_KEY_SEGMENTS: return False if not all(segments[:-1]): return False + if any(segment in _TRAVERSAL_SEGMENTS for segment in segments): + return False if segments[_LEGACY_KIND_INDEX] not in _ARTIFACT_KIND_VALUES: return False return bool(_CACHE_LEAF_RE.fullmatch(segments[-1])) diff --git a/src/cfdb/workflows/processors/base.py b/src/cfdb/workflows/processors/base.py index 35902f0..90f5bc5 100644 --- a/src/cfdb/workflows/processors/base.py +++ b/src/cfdb/workflows/processors/base.py @@ -83,18 +83,47 @@ def __init_subclass__(cls, **kwargs: Any) -> None: declaration at all and takes the class-name default, because an empty identity is the same failure as a missing one. - A declared value is validated here rather than at first use, so a - malformed identity (``" "``, ``".."``, one colliding with an - artifact kind) raises when the module is imported instead of - surfacing per-request inside a worker, long after the class that - caused it was written. + The identity is validated here rather than at first use, so a + malformed one (``" "``, ``".."``, one colliding with an artifact + kind) raises when the module is imported instead of surfacing + per-request inside a worker, long after the class that caused it + was written. The class-name default goes through the same + normalizer as a declared value: ``is_legacy_cache_key``'s safety + argument rests on no identity ever equalling an artifact kind, and + a guarantee that held only for declared identities would leave + ``class index(Processor)`` failing at derivation instead. + + A ``processor_id`` supplied by a **mixin** — a base that is not + itself a ``Processor`` — raises rather than being silently + discarded. Replacing it would be a lie the reader cannot see: the + mixin's source shows a pinned identity and the runtime uses the + class name, so factoring a pinned identity into a mixin would cold- + cache everything keyed under it with no signal. Inheriting from + another ``Processor`` is a different case and stays legal, because + every level of such a chain correctly takes its own class name. """ super().__init_subclass__(**kwargs) declared = cls.__dict__.get("processor_id") if not declared: - cls.processor_id = cls.__name__ - else: - cls.processor_id = key_utils.normalize_processor_id(declared) + cls._reject_mixin_supplied_identity() + cls.processor_id = key_utils.normalize_processor_id( + declared or cls.__name__ + ) + + @classmethod + def _reject_mixin_supplied_identity(cls) -> None: + """Raise when a non-``Processor`` base declares ``processor_id``.""" + for base in cls.__mro__[1:]: + if issubclass(base, Processor): + continue + supplied = base.__dict__.get("processor_id") + if supplied: + raise ValueError( + f"{cls.__name__} inherits processor_id {supplied!r} from " + f"mixin {base.__name__}, which would be silently discarded " + f"in favour of the class name. Declare processor_id in " + f"{cls.__name__}'s own body instead." + ) def artifact_kinds_produced( self, file_meta: dict[str, Any] | None = None diff --git a/src/cfdb/workflows/processors/tabix.py b/src/cfdb/workflows/processors/tabix.py index 1b86c61..e7920fa 100644 --- a/src/cfdb/workflows/processors/tabix.py +++ b/src/cfdb/workflows/processors/tabix.py @@ -163,11 +163,12 @@ def _read_prefix(path: Path, n: int = _SOURCE_SNIFF_BYTES) -> bytes: class TabixIntervalProcessor(Processor): """Handle plain-text genomic interval formats and produce a tabix index.""" + processor_id = "tabix-interval" + # v2: the source-encoding guard (issue #69) changed which sources will # ever be committed, so re-key all tabix artifacts — a poisoned v1 # ``data`` entry (committed before the guard existed) becomes a cache # miss, re-enters _stage_prepare, and is rejected instead of served. - processor_id = "tabix-interval" processor_version = 2 supported_formats = frozenset(_TABIX_PRESET.keys()) artifact_kinds = (ArtifactKind.DATA, ArtifactKind.INDEX) From 5235e47ecbf0889fd3d36e0d74b92ba93c308df8 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:43:27 -0400 Subject: [PATCH 08/14] fixup! feat: Reject two processors that share an identity at registration --- src/cfdb/workflows/processors/registry.py | 52 +++++++++++++++++++---- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/src/cfdb/workflows/processors/registry.py b/src/cfdb/workflows/processors/registry.py index 0255a7b..e7874a2 100644 --- a/src/cfdb/workflows/processors/registry.py +++ b/src/cfdb/workflows/processors/registry.py @@ -2,11 +2,25 @@ from __future__ import annotations +import unicodedata from typing import Any -from cfdb.workflows.processors.tools import format_name from cfdb.workflows.processors.base import Processor from cfdb.workflows.processors.passthrough import PassthroughProcessor +from cfdb.workflows.processors.tools import format_name + + +def _identity_fold(processor_id: str) -> str: + """Fold an identity to the form two cache entries would collide under. + + ``cache_key`` preserves an identity's case and Unicode spelling, but a + cache backend need not: the identity segment becomes a directory name + under ``LocalFsCache``, and a case-insensitive filesystem (APFS by + default) or a normalizing one folds two spellings onto one directory. + Comparing on the folded form is what makes the registry's uniqueness + guard match the guarantee ``cache_key`` is relied on to provide. + """ + return unicodedata.normalize("NFC", processor_id).casefold() class ProcessorRegistry: @@ -31,6 +45,20 @@ def register(self, processor: Processor) -> None: the file's format name, so callers should register more specific processors before more general ones. + The guard's scope is **this registry instance**, not the process: + uniqueness is a property of one wired deployment, and the single + wiring site is ``cfdb.api.main``'s lifespan. A deployment that + wires a second registry (a worker-side one, a batch tool) has to + re-establish the invariant itself — it travels with the registry, + not with the processor class. + + Identities are compared case- and Unicode-folded rather than by + exact string equality. ``cache_key`` preserves the raw spelling, + but a cache backend need not keep two spellings apart: on a + case-insensitive filesystem ``BedProcessor`` and ``BEDProcessor`` + derive distinct keys that resolve to one directory, which is the + aliasing this guard exists to prevent. + Raises: ValueError: Another registered processor already claims this one's ``processor_id``. Cache keys are scoped by that @@ -40,14 +68,22 @@ def register(self, processor: Processor) -> None: the property into an enforced invariant instead of a convention each new processor has to remember. """ + folded = _identity_fold(processor.processor_id) for registered in self._processors: - if registered.processor_id == processor.processor_id: - raise ValueError( - f"processor_id {processor.processor_id!r} is already " - f"registered by {type(registered).__name__}; cache keys " - f"are scoped by this identity, so " - f"{type(processor).__name__} would alias its artifacts" - ) + if _identity_fold(registered.processor_id) != folded: + continue + collision = ( + "is already registered by" + if registered.processor_id == processor.processor_id + else "collides once case- and Unicode-folded with the one " + "registered by" + ) + raise ValueError( + f"processor_id {processor.processor_id!r} {collision} " + f"{type(registered).__name__} ({registered.processor_id!r}); " + f"cache keys are scoped by this identity, so " + f"{type(processor).__name__} would alias its artifacts" + ) self._processors.append(processor) def lookup_for(self, file_meta: dict[str, Any]) -> Processor | None: From efa2cb75e7504a1930e016bd7409b864a5a9a7d0 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:43:30 -0400 Subject: [PATCH 09/14] fixup! feat: Add a sweep for cache entries under the retired key scheme --- README.md | 27 ++++++++++--- src/cfdb/cli.py | 80 ++++++++++++++++++++++++++++++------- src/cfdb/workflows/purge.py | 56 ++++++++++++++++++++++++-- 3 files changed, 140 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 3a9271e..9a84345 100644 --- a/README.md +++ b/README.md @@ -877,9 +877,19 @@ The preprocessed artifact is the default response. Clients that want the raw ups | GTF | GTF→GFF3 + sort + bgzip + tabix | bgzipped GFF3 + TBI | | bigBed | bigBedToBed + sort + bgzip + tabix | bgzipped BED + TBI | -Cache keys have the shape `{dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{processor_version}` — for example `encode/ENCFF732YBO/index/tabix-interval/6fccbb438a046075cb438f84d0defe8d-v2`. They are content-addressed using each file's upstream `md5`, so a byte change upstream (with the sync pipeline refreshing `md5`) invalidates the cache automatically, and they carry the producing processor's identity (`Processor.processor_id`) so two processors claiming the same file and artifact kind can never read back each other's output. Without that segment a version number was the only thing separating processors — and `TabixIntervalProcessor` and `BamIndexProcessor` both sit at version 2, staying apart only because their `supported_formats` happen to be disjoint. A processor that declares no `processor_id` inherits its own class name; declare one explicitly when the identity should survive a class rename, since changing the string invalidates every artifact keyed under it. The declaration must sit in the processor's own class body — a value supplied by a base class or mixin is discarded in favour of the class name, so factoring a pinned identity into a mixin would silently cold-cache everything keyed under it. An identity may not be blank, contain a path separator, be `.` or `..`, or equal an artifact kind (`data`, `index`); each is rejected when the class is declared, and the last of those keeps a mis-specified purge prefix from reducing a live key to something shaped like a retired one. +Cache keys have the shape `{dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{processor_version}` — for example `encode/ENCFF732YBO/index/tabix-interval/6fccbb438a046075cb438f84d0defe8d-v2`. They are content-addressed using each file's upstream `md5`, so a byte change upstream (with the sync pipeline refreshing `md5`) invalidates the cache automatically, and they carry the producing processor's identity (`Processor.processor_id`) so two processors claiming the same file and artifact kind can never read back each other's output. Without that segment a version number was the only thing separating processors — and `TabixIntervalProcessor` and `BamIndexProcessor` both sit at version 2, staying apart only because their `supported_formats` happen to be disjoint. A processor that declares no `processor_id` takes its own class name; declare one explicitly when the identity should survive a class rename, since changing the string invalidates every artifact keyed under it. The declaration must sit in the processor's own class body. Omitting it entirely — or writing `processor_id = ""` — takes the class-name default, which is safe because every class name is distinct. Supplying one from a base class or mixin instead raises at class definition: the value would be discarded in favour of the class name, so a mixin's pinned identity would read as authoritative in source while silently cold-caching everything keyed under it. Subclassing another processor stays legal, and each level of the chain takes its own class name. -**Purging the retired key scheme.** Keys minted before the processor-identity segment existed are unreachable by construction — the API derives the current shape and never probes the old one. Sweep them with `cfdb purge-legacy-cache` (dry run by default; pass `--apply` to delete). The same sweep clears the orphaned `.bedpe` / `bigInteract` index artifacts stranded when those formats were re-typed. Note that the deploy shipping the key change starts against a **fully cold cache**: every `/data` and `/index` request for a processable file dispatches a fresh workflow until the fleet catches up. +Whatever the identity ends up being — declared or defaulted — it must match `[A-Za-z0-9._-]`, and may not be `.`, `..`, or an artifact kind (`data`, `index`). Each is rejected when the class is declared, so a malformed identity fails on import rather than per-request inside a worker. The alphabet is an allowlist because a denylist of the separators still admits a newline, a zero-width space, or a fullwidth solidus — each invisible in review while addressing a different cache entry. The artifact-kind rule is what keeps a mis-specified purge prefix from reducing a live key to something shaped like a retired one. Note that case is preserved in the key but *not* relied on to keep two identities apart on disk: a case-insensitive filesystem folds `BedProcessor` and `BEDProcessor` onto one directory, so `ProcessorRegistry.register` refuses the pair. + +**Deploying the key change.** The cache key is derived independently in two separately-deployed processes: the API derives it to probe the cache, and the worker derives it to write the artifact. `processor_id` travels as a class attribute, so each resolves it from *its own* image. **Deploy the worker task definition at or before the API.** Workers are standalone `RunTask` tasks rather than an ECS service, so a stack update does not stop the ones already running — they exit only on `CFDB_WORKER_MAX_LIFETIME_SECONDS` (default 5 hours), and worker discovery gates on the wool protocol version, not the cfdb image. A new API dispatching to an old worker gets a job that completes having written a key the API will never probe, so the next request misses and dispatches again, indefinitely and silently. The same applies in reverse to an old API and a new worker. + +The deploy also starts against a **fully cold cache**: every `/data` and `/index` request for a processable file dispatches a fresh workflow until the cache refills. Re-keying the existing artifacts in place (`CopyObject` on S3, `rename` locally) was considered and rejected — it needs each artifact's producing processor resolved from file metadata, which is more moving parts than a cold start costs. + +**Rolling back past this change.** Reverting puts the system through a *second* cold start, since the API returns to deriving four-segment keys against a cache from which exactly those were swept. Every five-segment key written between deploy and revert becomes unreachable, and `cfdb purge-legacy-cache` will not clear it — the sweep only recognises the retired four-segment shape. On S3 the bucket lifecycle rule reclaims that residue within `CacheArtifactExpirationDays`; on a local cache root nothing does. + +**Purging the retired key scheme.** Keys minted before the processor-identity segment existed are unreachable by construction — the API derives the current shape and never probes the old one. Sweep them with `cfdb purge-legacy-cache` (dry run by default; pass `--apply` to delete). The same sweep clears the orphaned `.bedpe` / `bigInteract` index artifacts stranded when those formats were re-typed. + +Two things to know before running it against S3. First, the sweep needs `s3:ListBucket` on the bucket and `s3:DeleteObject` on `/*` — **neither the API nor the worker task role carries a delete grant**, deliberately, so run it under an operator or CI principal rather than from inside a deployed container. Second, the cache bucket already carries an `expire-cached-artifacts` lifecycle rule (`CacheArtifactExpirationDays`, default 30), and its rationale is exactly this case: keys are content-addressed, so a missing artifact is simply re-materialized. The retired population therefore self-reclaims within a month on its own. Sweeping S3 *accelerates* that reclaim; it is not required for correctness, and waiting is a legitimate choice. A local cache root has no equivalent expiry, so there the sweep is the only thing that reclaims. Run the local sweep with the API stopped — it prunes directories as it empties them, which can race a concurrent cache write. **Bounded concurrency, durable queuing, and admission control.** Dispatch is bounded on three cooperating layers so an unauthenticated burst on `/data` and `/index` can't oversubscribe the worker fleet or queue unbounded work: @@ -1000,7 +1010,7 @@ Poll the status of a dispatched workflow: "job_id": "abc-123", "status": "running", "stages_done": ["data"], - "artifacts": {"data": "encode/ENCFF123/data/abc-v1"}, + "artifacts": {"data": "encode/ENCFF123/data/tabix-interval/6fccbb438a046075cb438f84d0defe8d-v2"}, "progress": null, "error": null, "superseded_by": null @@ -1070,6 +1080,8 @@ Check the status of a sync task. The `task_id` is returned when starting a sync. ### CLI +#### `cfdb sync` + ```bash # Sync all DCCs cfdb sync @@ -1083,6 +1095,8 @@ cfdb sync 4dn hubmap - `--api-key` - API key for sync endpoint (env: `SYNC_API_KEY`) - `--debug` / `-d` - Enable debugpy debugging +#### `cfdb purge-legacy-cache` + ```bash # Report what the retired cache-key scheme is still holding (dry run) cfdb purge-legacy-cache @@ -1097,9 +1111,12 @@ cfdb purge-legacy-cache --s3-bucket cfdb-cache --s3-prefix dev --apply - `--s3-prefix` - key prefix the S3 cache backend writes under (env: `WORKFLOW_S3_PREFIX`) - `--endpoint-url` - boto3 endpoint override for LocalStack-backed dev (env: `AWS_ENDPOINT_URL`) - `--region` - AWS region for the boto3 client (env: `AWS_REGION`) -- `--local-root` - local cache root (default: `$SYNC_DATA_DIR/cache`) +- `--local-root` - local cache root, which must exist (default: `$SYNC_DATA_DIR/cache`) - `--apply` - actually delete; without it the sweep only reports +- `--yes` - skip the `--apply` confirmation prompt, for scripted runs + +Exactly one store is purged per run. When both an S3 bucket and a local root resolve — including the environment-only pairing of `WORKFLOW_S3_BUCKET` and `SYNC_DATA_DIR`, which a deployed API task has — the command refuses rather than guessing which cache you meant. The target is printed before the sweep begins, and `--apply` prompts for confirmation, because the store is chosen partly from ambient environment and the deletion cannot be undone. -Exactly one store is purged per run. When both an S3 bucket and a local root resolve — `WORKFLOW_S3_BUCKET` set in the environment alongside an explicit `--local-root`, say — the command refuses rather than guessing which cache you meant. +Pass `--s3-prefix` exactly as `WORKFLOW_S3_PREFIX` is set for the environment. A prefix carrying extra segments is rejected by the artifact-kind check rather than acted on, and one that is too short matches nothing — the command warns when it scanned entries and matched none, so a prefix typo does not read as an already-swept environment. `--s3-prefix` must be exactly the prefix the cache backend writes under (`WORKFLOW_S3_PREFIX`). A prefix carrying extra segments is stripped from every key before the retired-shape test, so an over-specified one would otherwise reduce live five-segment keys to four-segment ones; the sweep rejects those because the processor identity lands where an artifact kind must be, but the safest habit is still to pass the same value the API runs with. A prefix that is too short simply matches nothing. diff --git a/src/cfdb/cli.py b/src/cfdb/cli.py index 28975a5..3d9f851 100644 --- a/src/cfdb/cli.py +++ b/src/cfdb/cli.py @@ -117,31 +117,35 @@ def sync(dcc_names: tuple[str, ...], api_url: str, api_key: str): "--s3-bucket", default=None, envvar="WORKFLOW_S3_BUCKET", + show_envvar=True, help="Bucket holding the workflow cache (S3 profile).", ) @click.option( "--s3-prefix", default="", envvar="WORKFLOW_S3_PREFIX", + show_envvar=True, help="Key prefix the S3 cache backend writes under.", ) @click.option( "--endpoint-url", default=None, envvar="AWS_ENDPOINT_URL", + show_envvar=True, help="boto3 endpoint override (LocalStack-backed dev).", ) @click.option( "--region", default=None, envvar="AWS_REGION", + show_envvar=True, help="AWS region for the boto3 client.", ) @click.option( "--local-root", default=None, help="Local cache root. Defaults to $SYNC_DATA_DIR/cache.", - type=click.Path(file_okay=False, path_type=Path), + type=click.Path(file_okay=False, exists=True, path_type=Path), ) @click.option( "--apply", @@ -149,6 +153,12 @@ def sync(dcc_names: tuple[str, ...], api_url: str, api_key: str): help="Delete the matched entries. Without it the sweep is a dry run.", is_flag=True, ) +@click.option( + "--yes", + default=False, + help="Skip the --apply confirmation prompt (for scripted runs).", + is_flag=True, +) def purge_legacy_cache( s3_bucket: str | None, s3_prefix: str, @@ -156,6 +166,7 @@ def purge_legacy_cache( region: str | None, local_root: Path | None, apply: bool, + yes: bool, ): """ Sweep workflow cache entries minted under the retired key scheme. @@ -166,8 +177,14 @@ def purge_legacy_cache( entries, including the orphaned .bedpe / bigInteract index artifacts left behind when PR #108 re-typed those formats. - Runs as a DRY RUN unless --apply is passed. The target is the S3 - bucket when one is configured, otherwise the local cache root. + WARNING: --apply deletes objects irreversibly. Runs as a DRY RUN + unless it is passed. + + Exactly one store is swept per run. Resolving both an S3 bucket and a + local cache root is a usage error rather than a precedence rule -- + purging the wrong store cannot be undone, so the command refuses to + guess. $SYNC_DATA_DIR is consulted for the local root only when no + bucket is configured. Examples: @@ -179,18 +196,21 @@ def purge_legacy_cache( """ from cfdb.workflows.purge import build_s3_client, purge_local, purge_s3 - if local_root is None and not s3_bucket: - sync_data_dir = os.getenv("SYNC_DATA_DIR") - if sync_data_dir: - local_root = Path(sync_data_dir) / "cache" + sync_data_dir = os.getenv("SYNC_DATA_DIR") + env_local_root = Path(sync_data_dir) / "cache" if sync_data_dir else None + if local_root is None: + local_root = env_local_root if s3_bucket and local_root is not None: - # Both stores resolved — refuse rather than guess. WORKFLOW_S3_BUCKET - # in the environment is enough to trigger this alongside an explicit - # --local-root, and purging the wrong store is not recoverable. + # Both stores resolved — refuse rather than guess. This fires on the + # environment-only pairing too: a container that sets both + # WORKFLOW_S3_BUCKET and SYNC_DATA_DIR (backend.yml does) would + # otherwise silently sweep S3 for an operator who meant the local + # cache, and purging the wrong store is not recoverable. + source = "--local-root" if env_local_root != local_root else "$SYNC_DATA_DIR" raise click.UsageError( - "Both an S3 bucket and a local cache root resolved; pass only " - "one (unset WORKFLOW_S3_BUCKET to target --local-root)" + f"Both an S3 bucket and a local cache root ({source}) resolved; " + f"pass only one (unset WORKFLOW_S3_BUCKET to target the local root)" ) if not s3_bucket and local_root is None: raise click.UsageError( @@ -198,8 +218,23 @@ def purge_legacy_cache( "WORKFLOW_S3_BUCKET / SYNC_DATA_DIR" ) + target = ( + f"s3://{s3_bucket}/{s3_prefix.strip('/')}".rstrip("/") + if s3_bucket + else str(local_root) + ) + + # Name the target BEFORE sweeping. The store is chosen partly from + # ambient environment, so an operator must be able to see which one was + # picked while the run is still stoppable — not after the deletes. + click.echo(f"Target: {target}") + if apply and not yes: + click.confirm( + f"Irreversibly delete legacy cache entries from {target}?", + abort=True, + ) + if s3_bucket: - target = f"s3://{s3_bucket}/{s3_prefix.strip('/')}".rstrip("/") report = purge_s3( build_s3_client(endpoint_url=endpoint_url, region_name=region), s3_bucket, @@ -207,10 +242,8 @@ def purge_legacy_cache( apply=apply, ) else: - target = str(local_root) report = purge_local(local_root, apply=apply) - click.echo(f"Target: {target}") click.echo(f"Scanned: {report.scanned}") click.echo( f"Legacy entries: {report.matched} " @@ -221,6 +254,23 @@ def purge_legacy_cache( else: click.echo("Dry run — nothing deleted. Re-run with --apply.") + # A clean sweep and a mis-targeted one both report zero. Distinguish + # them, so an operator working through the migration runbook cannot tick + # an environment off on the strength of a prefix typo. + if report.scanned == 0: + click.echo( + f"WARNING: {target} held nothing — check the bucket, prefix, or " + f"path before treating this environment as swept.", + err=True, + ) + elif report.matched == 0: + click.echo( + f"WARNING: scanned {report.scanned} entries and matched none. If " + f"this environment was not already swept, check --s3-prefix " + f"against the deployment's WORKFLOW_S3_PREFIX.", + err=True, + ) + if __name__ == "__main__": cli() diff --git a/src/cfdb/workflows/purge.py b/src/cfdb/workflows/purge.py index 16bcf84..d9bd29e 100644 --- a/src/cfdb/workflows/purge.py +++ b/src/cfdb/workflows/purge.py @@ -5,7 +5,21 @@ (``{dcc}/{local_id}/{artifact_kind}/{md5}-v{processor_version}``) is therefore unreachable by construction: the router derives the new five-segment form and probes that, so the old entries are never read -again and never overwritten. They are pure storage cost until swept. +again and never overwritten. + +How much that costs depends on the backend, and the two differ: + +- ``LocalFsCache`` has no expiry of any kind, so a retired entry is a + permanent cost. This sweep is the only thing that reclaims it. +- The deployed S3 cache bucket already carries an ``expire-cached-artifacts`` + lifecycle rule (``cloudformation/workers.yml``) with + ``CacheArtifactExpirationDays``, default 30 — and its own rationale is + this one: keys are content-addressed, so expiry is safe because a missing + artifact is simply re-materialized. The retired population therefore + self-reclaims within a month with no operator action. Sweeping S3 + *accelerates* that reclaim; it is not required for correctness, and an + operator weighing an irreversible mass-delete against waiting should know + waiting is supported. The sweep also clears the orphaned paired-interval artifacts left by PR #108 — the incorrect ``.tbi`` files built for ``.bedpe`` / ``bigInteract`` @@ -17,6 +31,15 @@ The single description of the retired shape lives in :func:`cfdb.workflows.keys.is_legacy_cache_key`; nothing here re-derives it. + +**This module is temporary.** It exists to carry one migration across one +deploy of every environment. Once each environment has been swept, this +module, the ``cfdb purge-legacy-cache`` command, +:func:`~cfdb.workflows.keys.is_legacy_cache_key`, and the retired-scheme +constants beside it should all be deleted — along with the artifact-kind +rejection in :func:`~cfdb.workflows.keys.normalize_processor_id`, which +exists only to keep an over-stripped prefix from looking like a retired +key and has no bearing on the key itself. """ from __future__ import annotations @@ -101,8 +124,15 @@ def purge_s3( continue report.matched += 1 report.bytes_matched += obj.get("Size", 0) + # Only the applied path consumes ``batch``. Accumulating on a + # dry run would hold one string per matched object for the whole + # sweep and then discard them — and the dry run is both the + # default and the one an operator points at the largest, + # least-swept cache first. + if not apply: + continue batch.append(key) - if apply and len(batch) >= _S3_DELETE_BATCH: + if len(batch) >= _S3_DELETE_BATCH: report.deleted += _delete_s3_batch(client, bucket, batch) batch = [] @@ -159,13 +189,26 @@ def purge_local(root: Path, *, apply: bool = False) -> PurgeReport: A :class:`PurgeReport` for the run. Directories the deletions emptied are pruned so the tree does not retain the shape of the retired scheme. + + Run this with the API stopped. The pruning below removes a directory + the moment its last entry goes, which can land between a concurrent + ``LocalFsCache.put``'s ``mkdir(parents=True)`` and its ``os.replace`` + and fail that workflow. The window is narrow and the sweep is a + one-off migration step, so quiescing is cheaper than coordinating. """ report = PurgeReport() if not root.is_dir(): return report emptied: list[Path] = [] - for path in sorted(root.rglob("*")): + # ``rglob`` does not descend symlinked directories, which is what keeps + # the sweep inside ``root``: a symlinked cache subdirectory would + # otherwise let an irreversible delete reach arbitrary paths. Pinned by + # ``test_purge_local_should_not_delete_through_a_symlinked_directory``. + # Iterated lazily — the result is order-independent (matching is + # per-path and pruning is deferred to ``emptied``), so sorting would + # only force the whole tree into memory first. + for path in root.rglob("*"): if not path.is_file(): continue report.scanned += 1 @@ -191,6 +234,13 @@ def _prune_empty_ancestors(directory: Path, root: Path) -> None: ``$SYNC_DATA_DIR/cache`` is operator-supplied, so an unrelated empty directory under it is not the sweep's to reclaim. Stops at the first ancestor that still holds something, and never removes ``root``. + + The ``root in current.parents`` half of the loop condition is + unreachable for every caller: ``directory`` is always ``path.parent`` + for a ``path`` yielded by ``root.rglob``, so the walk is inside ``root`` + by construction and ``current != root`` alone terminates it. It is kept + as a containment assertion for an irreversible delete — do not write a + test for the state it guards, because no caller can reach it. """ current = directory while current != root and root in current.parents: From 9352cd6183505b8840adf0d80b6ad27dcff33d2a Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:43:34 -0400 Subject: [PATCH 10/14] fixup! test: Cover the processor identity and the retired-key predicate --- tests/test_workflows/test_keys.py | 172 +++++++++++++++++- tests/test_workflows/test_processors_base.py | 67 ++++++- .../test_processors_registry.py | 49 +++++ 3 files changed, 270 insertions(+), 18 deletions(-) diff --git a/tests/test_workflows/test_keys.py b/tests/test_workflows/test_keys.py index fe7a2f4..4de6084 100644 --- a/tests/test_workflows/test_keys.py +++ b/tests/test_workflows/test_keys.py @@ -16,6 +16,9 @@ workflow_key, ) from cfdb.workflows.models import ArtifactKind +from cfdb.workflows.processors.bam import BamIndexProcessor +from cfdb.workflows.processors.passthrough import PassthroughProcessor +from cfdb.workflows.processors.tabix import TabixIntervalProcessor from tests.test_workflows import FIXTURE_MD5 #: Mixed-case variant used to exercise normalization round-trips. @@ -35,18 +38,18 @@ _ARTIFACT_KIND_STRATEGY = st.sampled_from(list(ArtifactKind)) _VERSION_STRATEGY = st.integers(min_value=0, max_value=9_999) -#: Processor identities drawn from the vocabulary the shipped ids use -#: (letters, digits, and the ``-``/``_``/``.`` joiners), excluding the -#: values ``normalize_processor_id`` reserves. +#: Processor identities drawn from exactly the alphabet +#: ``normalize_processor_id`` admits — ASCII letters, digits, and the +#: ``-``/``_``/``.`` joiners — excluding the values it reserves. Drawing +#: from the Unicode letter category instead would generate identities the +#: normalizer rejects (``"ª"``), turning a property about key *content* +#: into one about key *validity*. _PROCESSOR_ID_STRATEGY = st.text( - alphabet=st.characters( - whitelist_categories=("L", "N"), whitelist_characters="-_." - ), + alphabet="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_.", min_size=1, max_size=24, ).filter( - lambda value: value.strip() == value - and value not in (".", "..") + lambda value: value not in (".", "..") and value not in {kind.value for kind in ArtifactKind} ) @@ -68,6 +71,25 @@ def test_normalize_dcc_should_strip_whitespace_and_lowercase(): assert normalize_dcc("4DN_DCIC") == "4dn_dcic" +@pytest.mark.parametrize("dcc", [".", ".."]) +def test_normalize_dcc_should_raise_when_path_traversal(dcc): + """Test that a traversal dcc is rejected at derivation. + + Given: + A dcc of ``.`` or ``..``. + When: + normalize_dcc is called. + Then: + It should raise ValueError, for the same reason + normalize_local_id does — the value becomes the leading segment + of every key derived for that source, and the local backend + silently collapses ``.`` out of the resulting path. + """ + # Act & assert + with pytest.raises(ValueError, match="traversal"): + normalize_dcc(dcc) + + def test_normalize_md5_should_strip_whitespace_and_lowercase(): """Test that normalize_md5 canonicalizes hex digests. @@ -193,6 +215,25 @@ def test_normalize_local_id_should_raise_when_empty(self): with pytest.raises(ValueError, match="local_id"): normalize_local_id("") + @pytest.mark.parametrize("local_id", [".", ".."]) + def test_normalize_local_id_should_raise_when_path_traversal(self, local_id): + """Test that a traversal local_id is rejected at derivation. + + Given: + A local_id of ``.`` or ``..``. + When: + normalize_local_id is called. + Then: + It should raise ValueError. This is the one key component + that comes from third-party DCC metadata, and the cache + backend catches only ``..`` — ``.`` is silently collapsed by + path resolution, landing one logical key at five segments on + S3 and four on disk. + """ + # Act & assert + with pytest.raises(ValueError, match="traversal"): + normalize_local_id(local_id) + class TestNormalizeProcessorId: def test_normalize_processor_id_should_strip_whitespace_and_preserve_case(self): @@ -243,7 +284,7 @@ def test_normalize_processor_id_should_raise_when_processor_id_contains_backslas cannot escape into cache paths. """ # Act & assert - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="forbidden chars"): normalize_processor_id("tabix\\interval") def test_normalize_processor_id_should_raise_when_processor_id_contains_null_byte( @@ -260,9 +301,68 @@ def test_normalize_processor_id_should_raise_when_processor_id_contains_null_byt cache path or a shell pipeline argument. """ # Act & assert - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="forbidden chars"): normalize_processor_id("tabix\x00interval") + @pytest.mark.parametrize( + "processor_id", + [ + pytest.param("tabix\ninterval", id="newline"), + pytest.param("tabix\x01interval", id="control-char"), + pytest.param("tabix​interval", id="zero-width-space"), + pytest.param("tabix‮interval", id="rtl-override"), + pytest.param("tabix／interval", id="fullwidth-solidus"), + pytest.param("tabix∕interval", id="division-slash"), + pytest.param("tabix%2Finterval", id="percent-encoded-slash"), + pytest.param("tabix interval", id="inner-space"), + ], + ) + def test_normalize_processor_id_should_raise_when_outside_the_allowlist( + self, processor_id + ): + """Test that only the documented alphabet reaches a cache key. + + Given: + An identity carrying a character outside ``[A-Za-z0-9._-]``. + When: + normalize_processor_id is called. + Then: + It should raise ValueError. A denylist of the separators + would admit every one of these, and each is invisible in + review while addressing a different cache entry — a + zero-width space renders identically to the id beside it. + """ + # Act & assert + with pytest.raises(ValueError, match="forbidden chars"): + normalize_processor_id(processor_id) + + @pytest.mark.parametrize( + "processor_id", + [ + pytest.param(7, id="int"), + pytest.param(b"tabix-interval", id="bytes"), + pytest.param(["tabix-interval"], id="list"), + ], + ) + def test_normalize_processor_id_should_raise_when_not_a_string(self, processor_id): + """Test that a non-string identity raises ValueError, not AttributeError. + + Given: + An identity that is not a ``str`` — the shape a ``__slots__`` + member descriptor or a copy-pasted ``processor_version`` + takes. + When: + normalize_processor_id is called. + Then: + It should raise ValueError naming the type. Both request-path + callers catch ValueError to fall through to direct upstream + streaming, so an AttributeError escaping ``.strip()`` would + surface as a 500 instead of that fall-through. + """ + # Act & assert + with pytest.raises(ValueError, match="must be a str"): + normalize_processor_id(processor_id) + def test_normalize_processor_id_should_raise_when_empty(self): """Test that an empty processor id is rejected. @@ -873,6 +973,31 @@ def test_is_legacy_cache_key_should_require_a_real_artifact_kind(self, key): # Act & assert assert is_legacy_cache_key(key) is False + @pytest.mark.parametrize( + "key", + [ + pytest.param(f"encode/./data/{FIXTURE_MD5}-v2", id="dot-segment"), + pytest.param(f"encode/../data/{FIXTURE_MD5}-v2", id="dotdot-segment"), + pytest.param(f"./ENCFF1/data/{FIXTURE_MD5}-v2", id="leading-dot"), + ], + ) + def test_is_legacy_cache_key_should_reject_a_traversal_segment(self, key): + """Test that a shape the producer could never mint is not claimed. + + Given: + A four-segment key carrying a ``.`` or ``..`` segment. + When: + is_legacy_cache_key is called. + Then: + It should return False. The cache backend refused ``..`` at + put time, so the retired scheme provably never wrote such a + key — and purge_s3 deletes through delete_objects directly, + bypassing that validation, so the predicate is the only thing + standing between a foreign object and an irreversible delete. + """ + # Act & assert + assert is_legacy_cache_key(key) is False + def test_is_legacy_cache_key_should_return_false_for_an_over_stripped_current_key( self, ): @@ -990,3 +1115,30 @@ def test_is_legacy_cache_key_should_claim_every_retired_key( # Act & assert assert is_legacy_cache_key(retired) is True + + +class TestShippedProcessorIdentities: + def test_shipped_processor_ids_should_be_disjoint_from_artifact_kinds(self): + """Test that no shipped identity collides with an artifact kind. + + Given: + The three processors the API wires at startup. + When: + Their identities are compared against every ArtifactKind + value. + Then: + The two sets should be disjoint. normalize_processor_id + rejects the collision at class-definition time, so adding a + member to ArtifactKind that matches a shipped identity would + crash-loop the API on import — this pins the constraint in CI + instead, where the enum is edited. + """ + # Arrange + shipped = { + BamIndexProcessor.processor_id, + PassthroughProcessor.processor_id, + TabixIntervalProcessor.processor_id, + } + + # Act & assert + assert shipped.isdisjoint({kind.value for kind in ArtifactKind}) diff --git a/tests/test_workflows/test_processors_base.py b/tests/test_workflows/test_processors_base.py index db7f22a..d52ca81 100644 --- a/tests/test_workflows/test_processors_base.py +++ b/tests/test_workflows/test_processors_base.py @@ -237,6 +237,29 @@ class _KindNamed(Processor): async def run(self, file_meta, workdir, cache): yield Complete(artifacts={}) + def test_processor_id_should_raise_when_the_class_name_collides_with_a_kind(self): + """Test that the class-name default is validated too, not just declared ids. + + Given: + A Processor subclass declaring no identity whose class name + is itself an ArtifactKind value. + When: + The class is declared. + Then: + It should raise ValueError at definition. Validating only a + declared identity would leave is_legacy_cache_key's + over-stripped-prefix safety argument — which rests on no + identity ever equalling an artifact kind — conditional on + class-naming convention, and the failure would surface from + cache_key_for on every request inside a worker instead. + """ + # Act & assert + with pytest.raises(ValueError, match="artifact kind"): + + class index(Processor): + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + def test_processor_id_should_be_distinct_at_every_level_of_a_hierarchy(self): """Test that no two levels of an inheritance chain share an identity. @@ -267,19 +290,45 @@ class _Level3(_Level2): # Assert assert ids == {"_Level1", "_Level2", "_Level3"} - def test_processor_id_should_ignore_a_value_supplied_by_a_mixin(self): - """Test that only the class's own body can declare an identity. + def test_processor_id_should_raise_when_supplied_by_a_mixin(self): + """Test that a mixin cannot supply an identity silently. Given: A plain mixin declaring ``processor_id``, and a Processor - subclass inheriting from that mixin. + subclass that inherits from it without declaring its own. + When: + The subclass is declared. + Then: + It should raise ValueError naming both classes. The default + is read from the class's own ``__dict__`` rather than the + MRO, so the mixin's value would otherwise be discarded in + favour of the class name — a lie the reader cannot see, and + one that would cold-cache everything keyed under it. + """ + + # Arrange + class _IdentityMixin: + processor_id = "from-mixin" + + # Act & assert + with pytest.raises(ValueError, match=r"from-mixin.*_IdentityMixin"): + + class _Mixed(_IdentityMixin, Processor): + async def run(self, file_meta, workdir, cache): + yield Complete(artifacts={}) + + def test_processor_id_should_use_its_own_declaration_over_a_mixins(self): + """Test that a class declaring its own identity may still use a mixin. + + Given: + A plain mixin declaring ``processor_id``, and a Processor + subclass inheriting from it that declares its own. When: The subclass's attribute is read. Then: - It should be the subclass's class name, not the mixin's - value — the default is applied from the class's own - ``__dict__`` rather than the MRO, so factoring a pinned - identity into a mixin silently loses it. + It should be the subclass's own declared value. Only an + identity the class would silently lose is rejected, so a + mixin remains usable for everything other than the identity. """ # Arrange @@ -287,11 +336,13 @@ class _IdentityMixin: processor_id = "from-mixin" class _Mixed(_IdentityMixin, Processor): + processor_id = "own-identity" + async def run(self, file_meta, workdir, cache): yield Complete(artifacts={}) # Act & assert - assert _Mixed.processor_id == "_Mixed" + assert _Mixed.processor_id == "own-identity" def test_processor_id_should_not_be_inherited_by_a_subclass(self): """Test that subclassing a pinned processor mints a fresh identity. diff --git a/tests/test_workflows/test_processors_registry.py b/tests/test_workflows/test_processors_registry.py index d87b179..c98d98d 100644 --- a/tests/test_workflows/test_processors_registry.py +++ b/tests/test_workflows/test_processors_registry.py @@ -108,6 +108,30 @@ async def run( return {} +#: Identities differing only in case. ``normalize_processor_id`` accepts +#: both -- the registry is what refuses the pair, because the identity +#: segment becomes a directory name and case-insensitive filesystems fold +#: the two together. +class _CasePinned(Processor): + processor_id = "BedProcessor" + processor_version = 0 + supported_formats = frozenset({"BED"}) + artifact_kinds = (ArtifactKind.INDEX,) + + async def run(self, file_meta, workdir, cache_root): + return {} + + +class _CaseVariant(Processor): + processor_id = "BEDProcessor" + processor_version = 0 + supported_formats = frozenset({"BED"}) + artifact_kinds = (ArtifactKind.INDEX,) + + async def run(self, file_meta, workdir, cache_root): + return {} + + class TestProcessorRegistry: def test_lookup_for_should_return_matching_processor(self): """Test that lookup_for picks the processor claiming the file format. @@ -367,3 +391,28 @@ def test_default_registry_should_return_an_independent_registry_each_call(self): assert isinstance( second.lookup_for({"file_format": {"name": "BAM"}}), BamIndexProcessor ) + + +class TestIdentityFolding: + def test_register_should_raise_when_two_identities_differ_only_in_case(self): + """Test that case-variant identities are refused as a collision. + + Given: + A registry holding a processor, and one whose identity + differs from it only in case. + When: + register is called for the second. + Then: + It should raise ValueError. cache_key preserves case, so the + two derive distinct keys — but the identity segment is a + directory name, and a case-insensitive filesystem (APFS by + default) folds them onto one directory, so the second + processor reads back the first's artifacts as cache hits. + """ + # Arrange + registry = ProcessorRegistry() + registry.register(_CasePinned()) + + # Act & assert + with pytest.raises(ValueError, match="folded"): + registry.register(_CaseVariant()) From c3440efa14cb68c022e0ce3036936dc726856edf Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:43:37 -0400 Subject: [PATCH 11/14] fixup! test: Cover the purge sweep and its CLI --- tests/test_cli.py | 182 +++++++++++++++++++++++++++-- tests/test_workflows/test_purge.py | 32 +++++ 2 files changed, 205 insertions(+), 9 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index ba956d6..2214a01 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -59,9 +59,13 @@ def cache_root(tmp_path): return tmp_path -def _invoke(*args): - """Run ``cfdb purge-legacy-cache`` with ``args`` and return the result.""" - return CliRunner().invoke(cli, ["purge-legacy-cache", *args]) +def _invoke(*args, input: str | None = None): + """Run ``cfdb purge-legacy-cache`` with ``args`` and return the result. + + Tests exercising a sweep pass ``--yes`` to skip the ``--apply`` + confirmation; the prompt itself is covered by its own tests. + """ + return CliRunner().invoke(cli, ["purge-legacy-cache", *args], input=input) class TestPurgeLegacyCacheCommand: @@ -103,7 +107,7 @@ def test_purge_legacy_cache_should_delete_the_legacy_entry_when_applied( legacy entry. """ # Act - result = _invoke("--local-root", str(cache_root), "--apply") + result = _invoke("--local-root", str(cache_root), "--apply", "--yes") # Assert assert result.exit_code == 0 @@ -133,7 +137,7 @@ def test_purge_legacy_cache_should_refuse_when_both_stores_resolve( remote = mocker.patch.object(purge_module, "purge_s3") # Act - result = _invoke("--local-root", str(cache_root), "--apply") + result = _invoke("--local-root", str(cache_root), "--apply", "--yes") # Assert assert result.exit_code != 0 @@ -165,7 +169,7 @@ def test_purge_legacy_cache_should_refuse_when_no_store_resolves(self, mocker): local.assert_not_called() def test_purge_legacy_cache_should_fall_back_to_the_sync_data_dir( - self, cache_root, monkeypatch, tmp_path + self, monkeypatch, tmp_path ): """Test that SYNC_DATA_DIR resolves the documented default root. @@ -187,7 +191,7 @@ def test_purge_legacy_cache_should_fall_back_to_the_sync_data_dir( monkeypatch.setenv("SYNC_DATA_DIR", str(data_dir)) # Act - result = _invoke("--apply") + result = _invoke("--apply", "--yes") # Assert assert result.exit_code == 0 @@ -273,7 +277,7 @@ def test_purge_legacy_cache_should_sweep_the_configured_bucket_and_prefix( mocker.patch.object(purge_module, "build_s3_client", return_value=client) # Act - result = _invoke("--s3-bucket", _BUCKET, "--s3-prefix", "dev", "--apply") + result = _invoke("--s3-bucket", _BUCKET, "--s3-prefix", "dev", "--apply", "--yes") # Assert assert result.exit_code == 0 @@ -335,7 +339,7 @@ def test_purge_legacy_cache_should_resolve_every_option_from_the_environment( ) # Act - result = _invoke("--apply") + result = _invoke("--apply", "--yes") # Assert assert result.exit_code == 0 @@ -374,3 +378,163 @@ def test_purge_legacy_cache_should_render_the_reclaimable_size(self, mocker): # Assert assert "Scanned: 9" in result.output assert "Legacy entries: 4 (1,234,567,890 bytes, 1.15 GiB)" in result.output + + def test_purge_legacy_cache_should_refuse_when_both_stores_resolve_from_the_environment( + self, monkeypatch, mocker, tmp_path + ): + """Test that the ambiguity guard covers the environment-only pairing. + + Given: + WORKFLOW_S3_BUCKET and SYNC_DATA_DIR both exported and no + flags at all — the shape a deployed container has, since + backend.yml sets both on one task. + When: + The command is invoked with --apply. + Then: + It should exit non-zero and sweep neither store. Resolving the + local root only when no bucket is configured would let the + bucket win silently here, deleting from production for an + operator who meant their local cache. + """ + # Arrange + monkeypatch.setenv("WORKFLOW_S3_BUCKET", "prod-bucket") + monkeypatch.setenv("SYNC_DATA_DIR", str(tmp_path)) + local = mocker.patch.object(purge_module, "purge_local") + remote = mocker.patch.object(purge_module, "purge_s3") + + # Act + result = _invoke("--apply", "--yes") + + # Assert + assert result.exit_code != 0 + assert "Both an S3 bucket and a local cache root" in result.output + local.assert_not_called() + remote.assert_not_called() + + def test_purge_legacy_cache_should_name_the_target_before_sweeping( + self, cache_root, mocker + ): + """Test that the target is printed even when the sweep raises. + + Given: + A sweep that raises part-way, as a partial S3 delete failure + does. + When: + The command is invoked. + Then: + The target should already be in the output. The store is + chosen partly from ambient environment, so an operator must + be able to see which one was picked without waiting for a + sweep that may never return. + """ + # Arrange + mocker.patch.object( + purge_module, "purge_local", side_effect=RuntimeError("boom") + ) + + # Act + result = _invoke("--local-root", str(cache_root)) + + # Assert + assert result.exit_code != 0 + assert f"Target: {cache_root}" in result.output + + def test_purge_legacy_cache_should_abort_when_the_confirmation_is_declined( + self, cache_root + ): + """Test that --apply asks before deleting anything. + + Given: + A local cache root holding a legacy entry. + When: + The command is invoked with --apply and the prompt is + answered "n". + Then: + It should exit non-zero and leave the entry in place. The + flag is one word away from an irreversible mass delete, so it + gates on an explicit answer rather than on the flag alone. + """ + # Act + result = _invoke("--local-root", str(cache_root), "--apply", input="n\n") + + # Assert + assert result.exit_code != 0 + assert (cache_root / _LEGACY_KEY).exists() + + def test_purge_legacy_cache_should_warn_when_it_matched_nothing( + self, cache_root, mocker + ): + """Test that a mis-targeted sweep is distinguishable from a clean one. + + Given: + A sweep that scanned entries but matched none — the shape an + under-specified --s3-prefix produces. + When: + The command is invoked. + Then: + It should warn about the prefix. Reporting only "Legacy + entries: 0" would let an operator tick an environment off the + migration runbook on the strength of a typo. + """ + # Arrange + mocker.patch.object(purge_module, "build_s3_client", return_value=object()) + mocker.patch.object( + purge_module, "purge_s3", return_value=PurgeReport(scanned=12, matched=0) + ) + + # Act + result = _invoke("--s3-bucket", _BUCKET) + + # Assert + assert "matched none" in result.output + assert "WORKFLOW_S3_PREFIX" in result.output + + def test_purge_legacy_cache_should_warn_when_the_target_held_nothing( + self, cache_root, mocker + ): + """Test that an empty target is called out rather than reported clean. + + Given: + A sweep that scanned nothing at all — the shape a typo'd + bucket or an unwritten cache root produces. + When: + The command is invoked. + Then: + It should warn that the target held nothing. + """ + # Arrange + mocker.patch.object(purge_module, "build_s3_client", return_value=object()) + mocker.patch.object( + purge_module, "purge_s3", return_value=PurgeReport(scanned=0, matched=0) + ) + + # Act + result = _invoke("--s3-bucket", _BUCKET) + + # Assert + assert "held nothing" in result.output + + def test_purge_legacy_cache_should_reject_a_local_root_that_does_not_exist( + self, tmp_path, mocker + ): + """Test that a mistyped --local-root is a usage error. + + Given: + An explicit --local-root naming a directory that is not there. + When: + The command is invoked. + Then: + It should exit non-zero without sweeping. A typo would + otherwise produce a zeroed report indistinguishable from an + already-swept cache. + """ + # Arrange + local = mocker.patch.object(purge_module, "purge_local") + + # Act + result = _invoke("--local-root", str(tmp_path / "absent")) + + # Assert + assert result.exit_code != 0 + assert "does not exist" in result.output + local.assert_not_called() diff --git a/tests/test_workflows/test_purge.py b/tests/test_workflows/test_purge.py index 8a9de11..f0fb9b2 100644 --- a/tests/test_workflows/test_purge.py +++ b/tests/test_workflows/test_purge.py @@ -983,3 +983,35 @@ def test_purge_local_should_delete_exactly_what_cache_key_no_longer_mints( assert report.deleted == 1 assert not retired_path.exists() assert current_path.exists() + + def test_purge_local_should_not_delete_through_a_symlinked_directory( + self, tmp_path + ): + """Test that the sweep cannot reach outside its own cache root. + + Given: + A cache root containing a symlink to a directory outside it, + which itself holds a legacy-shaped entry. + When: + purge_local runs applied. + Then: + It should match nothing and leave the outside file intact. + Containment rests entirely on ``Path.rglob`` not descending + symlinked directories — an implicit default that, if it ever + changed, would turn an irreversible delete loose on arbitrary + paths. Pinned here rather than inherited. + """ + # Arrange + root = tmp_path / "cache" + root.mkdir() + outside = tmp_path / "outside" + _seed_local(outside, _LEGACY_KEY) + (root / "encode").symlink_to(outside / "encode", target_is_directory=True) + + # Act + report = purge_local(root, apply=True) + + # Assert + assert report.matched == 0 + assert report.deleted == 0 + assert (outside / _LEGACY_KEY).exists() From 41410fc7c98a6eb7326648959fa2cb07dc116af3 Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:43:40 -0400 Subject: [PATCH 12/14] fixup! test: Pin per-processor cache scoping across the router and executor --- tests/integration/test_processor_e2e.py | 29 +++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/tests/integration/test_processor_e2e.py b/tests/integration/test_processor_e2e.py index d648e18..242c6d4 100644 --- a/tests/integration/test_processor_e2e.py +++ b/tests/integration/test_processor_e2e.py @@ -32,6 +32,8 @@ from cfdb.workflows.keys import is_legacy_cache_key from cfdb.workflows.lock import get_job from cfdb.workflows.models import JobStatus +from cfdb.workflows.processors.bam import BamIndexProcessor +from cfdb.workflows.processors.tabix import TabixIntervalProcessor from tests.integration.conftest import ( CacheState, @@ -47,7 +49,7 @@ pytestmark = pytest.mark.integration -def _assert_production_key_shape(record, executor) -> None: +def _assert_production_key_shape(record, expected_processor_id: str) -> None: """Assert every persisted artifact key carries a processor identity. The e2e assertions elsewhere in this file join a cached path from @@ -56,12 +58,18 @@ def _assert_production_key_shape(record, executor) -> None: through a real worker, is made to prove it wrote under the current five-segment shape — and that the sweep would not claim what it just produced. + + MUST be called OUTSIDE the ``xfail_known_bugs`` body. That fixture + converts a matching exception into ``pytest.xfail``, so an + ``AssertionError`` raised inside the body cannot fail the run — this + assertion would silently become decorative. The identity is passed in + rather than looked up from the executor so the check pins which + processor ran, instead of agreeing with whatever the executor reports. """ - processor = executor._registry.lookup_for(record.file_meta_snapshot) for key in record.artifact_cache_keys.values(): segments = key.split("/") assert len(segments) == 5, key - assert segments[3] == processor.processor_id, key + assert segments[3] == expected_processor_id, key assert is_legacy_cache_key(key) is False, key @@ -186,7 +194,6 @@ async def _body(): assert final.status == JobStatus.COMPLETED assert final.stages_done == ["index"] assert "data" not in final.artifact_cache_keys - _assert_production_key_shape(final, integration_executor) cache_root = integration_executor._cache.root cached_bai = cache_root / final.artifact_cache_keys["index"] @@ -206,8 +213,10 @@ async def _body(): check=True, capture_output=True, ) + return final - await xfail_known_bugs(scenario, _body) + final = await xfail_known_bugs(scenario, _body) + _assert_production_key_shape(final, BamIndexProcessor.processor_id) @pytest.mark.asyncio async def test_ensure_workflow_should_convert_and_index_sam_input( @@ -294,7 +303,6 @@ async def _body(): # Assert final = await get_job(install_jobs_index, record.job_id) assert final is not None and final.status == JobStatus.COMPLETED - _assert_production_key_shape(final, integration_executor) cache_root = integration_executor._cache.root cached_bgz = cache_root / final.artifact_cache_keys["data"] @@ -324,8 +332,10 @@ async def _body(): text=True, ) assert query.stdout.count("\n") > 0 + return final - await xfail_known_bugs(scenario, _body) + final = await xfail_known_bugs(scenario, _body) + _assert_production_key_shape(final, TabixIntervalProcessor.processor_id) @pytest.mark.parametrize("scenario", _BED_LIKE_SCENARIOS, ids=str) @pytest.mark.asyncio @@ -377,7 +387,6 @@ async def _body(): # Assert final = await get_job(install_jobs_index, record.job_id) assert final is not None and final.status == JobStatus.COMPLETED - _assert_production_key_shape(final, integration_executor) cache_root = integration_executor._cache.root cached_bgz = cache_root / final.artifact_cache_keys["data"] @@ -394,8 +403,10 @@ async def _body(): text=True, ) assert query.stdout.count("\n") > 0 + return final - await xfail_known_bugs(scenario, _body) + final = await xfail_known_bugs(scenario, _body) + _assert_production_key_shape(final, TabixIntervalProcessor.processor_id) @pytest.mark.asyncio async def test_ensure_workflow_should_convert_gtf_to_gff3_and_index( From 2bc5651ac0fd6964be09dbd71568fbf2b5fa95af Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:44:14 -0400 Subject: [PATCH 13/14] fix: Await the jobs mutex index so the fake actually installs it install_jobs_index was a synchronous fixture calling FakeCollection.create_index, which is a coroutine matching Motor's API. The coroutine was never awaited, so the partial-unique index on workflow_key was never created and the mutex it backs was absent for every test that requested the fixture. The concurrent-dedup assertions in test_concurrency.py have therefore never exercised the mutex they name. They failed on two distinct job ids and were absorbed as expected failures by an unrelated known-bug entry, so the gap was invisible. With the index installed they pass on the real behaviour. --- tests/integration/conftest.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index e3f58c5..293eb53 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -567,10 +567,16 @@ def integration_workdir_root(tmp_path) -> Path: return root -@pytest.fixture() -def install_jobs_index(mock_db): - """Seed the partial-unique mutex index on the FakeDB jobs collection.""" - mock_db.jobs.create_index( +@pytest_asyncio.fixture() +async def install_jobs_index(mock_db): + """Seed the partial-unique mutex index on the FakeDB jobs collection. + + ``FakeCollection.create_index`` is a coroutine, matching Motor. This + fixture used to call it without awaiting, so the index was never + actually installed and every concurrent-dedup assertion below was + passing or failing for unrelated reasons. + """ + await mock_db.jobs.create_index( {"workflow_key": 1}, unique=True, partialFilterExpression={"active": True}, From 62f21d13bc42e4a35b3bec182173a6e275c1f2ff Mon Sep 17 00:00:00 2001 From: Conrad Date: Wed, 19 Aug 2026 21:44:21 -0400 Subject: [PATCH 14/14] test: Scope the tabix known-bug entry to the failure it names The tabix_macos_sigpipe_under_load entry listed AssertionError in both its raises tuple and its retryable predicate. It matches on format alone, covering nine of the eleven formats, so it absorbed every assertion failure in those scenarios on every platform and reported each as an expected failure caused by a subprocess SIGPIPE that may not have occurred. AssertionError is the type every assertion in the suite raises, so this made the affected tests unable to fail: deleting the processor identity segment from cache_key produced eight expected failures and exit code 0. Ten genuinely failing concurrency tests were being reported the same way. A known-bug entry has to name the failure it actually knows about, so the entry now matches only the RuntimeError carrying the SIGPIPE signature. --- tests/integration/conftest.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 293eb53..0167cc2 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -368,7 +368,13 @@ class _KnownBug: Format.SAM, } ), - raises=(RuntimeError, AssertionError), + # Scoped to the SIGPIPE signature alone. Listing ``AssertionError`` + # here would absorb every assertion failure in nine of the eleven + # formats, on every platform, and report it as this known bug — so + # a real regression would surface as an xfail and exit 0. That is + # the type every assertion in the suite raises; a known-bug entry + # must name the failure it actually knows about. + raises=(RuntimeError,), reason=( "macOS dev hosts intermittently SIGPIPE the tabix/samtools " "subprocess from inside a wool worker — grpc poll FDs " @@ -378,10 +384,8 @@ class _KnownBug: ), retries=3, retryable=lambda exc: ( - isinstance(exc, RuntimeError) - and "exited -13" in str(exc) - ) - or isinstance(exc, AssertionError), + isinstance(exc, RuntimeError) and "exited -13" in str(exc) + ), ), )