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 1452b2c..9a84345 100644 --- a/README.md +++ b/README.md @@ -877,7 +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 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` 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. + +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: @@ -998,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 @@ -1068,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 @@ -1080,3 +1094,29 @@ 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 + +#### `cfdb purge-legacy-cache` + +```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, 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. + +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 9de9f73..3d9f851 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,165 @@ 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", + 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, exists=True, path_type=Path), +) +@click.option( + "--apply", + default=False, + 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, + endpoint_url: str | None, + region: str | None, + local_root: Path | None, + apply: bool, + yes: 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. + + 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: + + 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 + + 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. 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( + 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( + "No cache to purge: pass --s3-bucket or --local-root, or set " + "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: + report = purge_s3( + build_s3_client(endpoint_url=endpoint_url, region_name=region), + s3_bucket, + prefix=s3_prefix, + apply=apply, + ) + else: + report = purge_local(local_root, apply=apply) + + 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.") + + # 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/keys.py b/src/cfdb/workflows/keys.py index 455d7aa..6c9a240 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,44 @@ _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+$") + +#: 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 +#: 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. +#: +#: 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``. @@ -33,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 @@ -61,15 +109,134 @@ 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 +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 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``. 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, 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. + """ + 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 not _PROCESSOR_ID_RE.fullmatch(cleaned): + raise ValueError(f"processor_id contains forbidden chars: {processor_id!r}") + 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( + 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. + + 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])) + + 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 +326,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 +337,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 +348,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 +358,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 +374,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..90f5bc5 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,60 @@ 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. + + 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._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 ) -> tuple[ArtifactKind, ...]: @@ -80,9 +150,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 +165,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/registry.py b/src/cfdb/workflows/processors/registry.py index bb633d1..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: @@ -30,7 +44,46 @@ 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. + + 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 + 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. """ + folded = _identity_fold(processor.processor_id) + for registered in self._processors: + 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: diff --git a/src/cfdb/workflows/processors/tabix.py b/src/cfdb/workflows/processors/tabix.py index 1c5af12..e7920fa 100644 --- a/src/cfdb/workflows/processors/tabix.py +++ b/src/cfdb/workflows/processors/tabix.py @@ -163,6 +163,8 @@ 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 diff --git a/src/cfdb/workflows/purge.py b/src/cfdb/workflows/purge.py new file mode 100644 index 0000000..d9bd29e --- /dev/null +++ b/src/cfdb/workflows/purge.py @@ -0,0 +1,256 @@ +"""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. + +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`` +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. + +**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 + +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) + # 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 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. + + 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] = [] + # ``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 + 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``. + + 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: + 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 diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index e3f58c5..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) + ), ), ) @@ -567,10 +571,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}, 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..242c6d4 100644 --- a/tests/integration/test_processor_e2e.py +++ b/tests/integration/test_processor_e2e.py @@ -29,8 +29,11 @@ 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 +from cfdb.workflows.processors.bam import BamIndexProcessor +from cfdb.workflows.processors.tabix import TabixIntervalProcessor from tests.integration.conftest import ( CacheState, @@ -46,6 +49,30 @@ pytestmark = pytest.mark.integration +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 + ``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. + + 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. + """ + for key in record.artifact_cache_keys.values(): + segments = key.split("/") + assert len(segments) == 5, key + assert segments[3] == expected_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. @@ -186,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( @@ -303,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 @@ -372,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( 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_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..2214a01 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,540 @@ +"""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, 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: + 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", "--yes") + + # 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", "--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_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, 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", "--yes") + + # 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", "--yes") + + # 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", "--yes") + + # 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 + + 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_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( diff --git a/tests/test_workflows/test_keys.py b/tests/test_workflows/test_keys.py index 6675a3d..4de6084 100644 --- a/tests/test_workflows/test_keys.py +++ b/tests/test_workflows/test_keys.py @@ -3,22 +3,56 @@ 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 +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. _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 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="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_.", + min_size=1, + max_size=24, +).filter( + lambda value: 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. @@ -37,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. @@ -162,6 +215,258 @@ 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): + """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, match="forbidden chars"): + 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, 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. + + 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): @@ -295,24 +600,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 +637,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 +680,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 +709,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 +723,422 @@ 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 + + @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, + ): + """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 + + +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_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..d52ca81 100644 --- a/tests/test_workflows/test_processors_base.py +++ b/tests/test_workflows/test_processors_base.py @@ -131,7 +131,248 @@ 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_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. + + 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_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 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 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 + 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 == "own-identity" + + 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 +381,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 +400,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..c98d98d 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"}) @@ -39,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. @@ -106,16 +199,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 +222,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 +332,87 @@ 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 + ) + + +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()) 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 diff --git a/tests/test_workflows/test_purge.py b/tests/test_workflows/test_purge.py new file mode 100644 index 0000000..f0fb9b2 --- /dev/null +++ b/tests/test_workflows/test_purge.py @@ -0,0 +1,1017 @@ +"""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() + + 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()