Fold a processor identity into the workflow cache key — Closes #109 - #110
Draft
conradbzura wants to merge 14 commits into
Draft
Fold a processor identity into the workflow cache key — Closes #109#110conradbzura wants to merge 14 commits into
conradbzura wants to merge 14 commits into
Conversation
cache_key identified the producing processor only by its version number,
never by which processor it is. Two processors claiming the same file and
artifact kind at equal processor_version therefore derived an identical
key and read back each other's artifacts as cache hits — a wrong answer
rather than a miss. Nothing collided, but only by accident of the
registry: TabixIntervalProcessor and BamIndexProcessor both sit at
version 2 and stayed apart solely because their supported_formats are
disjoint.
Keys now carry the producing processor's identity as its own segment:
{dcc}/{local_id}/{artifact_kind}/{processor_id}/{md5}-v{version}
Processor gains a processor_id class attribute, defaulted per subclass to
its own class name so a processor that declares nothing still derives
distinct keys — forgetting is safe rather than silently aliasing. The
three shipped processors pin explicit identities instead, so the values
survive a class rename; changing one invalidates every artifact keyed
under it. The default is read from the subclass's own __dict__ rather
than the MRO, because a subclass may emit different bytes at the same
version, and inheriting the parent's identity would recreate exactly the
aliasing the segment prevents.
An identity is validated when the class is declared rather than at first
use, so a malformed value fails on import instead of surfacing
per-request inside a worker. Beyond the separator and null-byte guards
normalize_local_id already applies, an identity may not be "." or ".."
— caught today only by the cache backend, a different module with a
different rationale — nor equal an artifact kind, which would let an
over-specified purge prefix strip a live key down to something shaped
exactly like a retired one.
is_legacy_cache_key describes the retired four-segment scheme for the
migration sweep. It requires a real artifact kind in the third segment,
not merely the right segment count: it gates an irreversible delete, so a
false positive is data loss while a false negative only leaves a stale
object behind. That check is also what makes a mis-specified purge prefix
safe, since stripping one segment too many leaves the processor identity
where an artifact kind must be.
The class-name default keeps processors that declare nothing apart, but it cannot catch the case that actually matters: two distinct classes pinning the same explicit processor_id, as a new processor copy-pasting a shipped identity would. Cache keys are scoped by that identity, so the two would read back each other's artifacts as cache hits. Rejecting at wiring time makes the property an enforced invariant rather than a convention every new processor has to remember, and fails in CI instead of at API startup. The guard keys on the identity alone, so two processors claiming one format remain legal and order-resolved — the documented first-registration-wins contract is unchanged.
Folding a processor identity into the cache key invalidates every existing artifact by construction — the API derives the five-segment shape and never probes the old one — so the entries are unreachable storage cost until swept. The sweep also clears the orphaned .bedpe and bigInteract index artifacts stranded when those formats were re-typed; they are retired-scheme keys too, so they need no separate pass. Both backends are covered because a deployment runs one or the other, and cfdb purge-legacy-cache exposes them through the installed console script so the sweep can run from a deployed image. It is a dry run unless --apply is passed, and it refuses to act when both an S3 bucket and a local root resolve rather than guessing which cache was meant — WORKFLOW_S3_BUCKET in the environment is enough to reach that state alongside an explicit --local-root, and purging the wrong store cannot be undone. Two failure modes are surfaced rather than absorbed. DeleteObjects reports per-key failures in the response body instead of raising, so a sweep that deleted nothing for want of an s3:DeleteObject grant would otherwise report success; and a response confirming fewer keys than were requested means something was neither deleted nor complained about. Both raise, and the sweep is idempotent, so re-running after fixing the cause picks up whatever is left. The local sweep prunes only the directories its own deletions emptied. The root is operator-supplied, so an unrelated empty directory under it is not the sweep's to reclaim.
Pins the property the issue asks to enforce: two processors claiming the same file and artifact kind at equal processor_version derive different keys. Covered as an example at both the keys and the Processor layer, as a Hypothesis property over the whole input domain, and against the shipped TabixIntervalProcessor and BamIndexProcessor pair the issue names as staying apart only by accident. is_legacy_cache_key gets the heaviest coverage, because it decides what an irreversible sweep deletes. Two properties bracket it: no key cache_key can mint is ever claimed, so a live artifact cannot be swept; and every key the retired scheme could mint is claimed, so the migration leaves no orphan. Around them sit the rejection cases — wrong segment counts, blank segments, foreign objects sharing the bucket, a workflow_key, and a current key stripped of its leading segment as an over-specified purge prefix produces. The shipped processor identities are asserted as literals rather than derived from the classes. They are wire constants: every cached artifact is keyed under one, the README documents "tabix-interval" inside an example key, and changing a string silently invalidates that processor's whole cached corpus. Deriving them would assert nothing. The duplicate-registration test previously registered one class twice, which the class-name default makes nearly unreachable. It now uses two distinct classes pinning one identity, and asserts the message names the identity and the incumbent, since that message is the operator's only diagnostic. The ordering test needed a second BAM-claiming class for the same reason, so it now also documents that the guard is scoped by identity rather than by format.
cfdb.cli had no test module at all, and this change adds a command to it that deletes production data irrecoverably. Both usage guards are now covered — the no-target case and the ambiguous case where an S3 bucket and a local root both resolve — and the ambiguity test asserts that neither sweep ran, not merely that the message was printed. An autouse fixture clears the five environment variables the options bind to. Every one of them leaks from a developer's shell, and without the fixture the ambiguity guard would fire on tests that never mention S3, passing in CI and failing locally. The sweep's boundaries get direct coverage: pagination across more than one listing page, the delete-request batch boundary, prefixes written bare or slashed, and a neighbouring environment whose prefix is a strict textual prefix of the swept one. The over-specified prefix is a regression test — it deletes a live artifact without the artifact-kind check in is_legacy_cache_key. The DeleteObjects error path uses a client double rather than moto, which populates Errors only for versioned deletes the sweep never issues. purge_s3 takes its client as a public parameter, so the double stays on the public surface. A Hypothesis property spans the two modules: for any file identity seeded under both schemes, the sweep deletes exactly the retired entry and leaves what cache_key mints. That is what catches the deriver and the sweep drifting apart in a future key change.
The identity property was pinned only at key derivation, never where a wrong answer would actually be served. The router now has to prove that an artifact cached under another processor's identity is a miss on GET, on HEAD, and on the readiness probe, and that an artifact sitting at a retired-scheme key is likewise unreachable — the latter being what makes the sweep safe to run and what leaves the shipping deploy cold. Several cache-hit tests had become tautologies. They rebuilt the expected key with the same expression cache_key_for evaluates, so they would have kept passing if the identity segment were dropped entirely. They now seed through the processor's own derivation, which is the agreement the router probe and the processor put actually depend on. Two fixtures held four-segment key literals under a comment claiming they matched what cache_key produces in production. That stopped being true, and the executor stub's keys reach real JobRecord.artifact_cache_keys in integration runs, so both are now derived. The executor also asserts the persisted keys equal what the processor derives, closing the chain from producer through job record to router probe. The end-to-end suites asserted only that a cached path existed, which holds under any key scheme. They now check the shape a real processor driven through a real worker actually wrote. test_lifespan_registry covers a hole the duplicate-identity guard made consequential: the existing lifespan test replaces default_registry with a MagicMock, so no test had ever run register against a registry already holding PassthroughProcessor. It also pins that startup builds a fresh registry, the one assignment keeping a re-entered lifespan from turning the guard into a boot crash-loop. Remaining changes are call-site updates for the new cache_key signature.
install_jobs_index was a synchronous fixture calling FakeCollection.create_index, which is a coroutine matching Motor's API. The coroutine was never awaited, so the partial-unique index on workflow_key was never created and the mutex it backs was absent for every test that requested the fixture. The concurrent-dedup assertions in test_concurrency.py have therefore never exercised the mutex they name. They failed on two distinct job ids and were absorbed as expected failures by an unrelated known-bug entry, so the gap was invisible. With the index installed they pass on the real behaviour.
The tabix_macos_sigpipe_under_load entry listed AssertionError in both its raises tuple and its retryable predicate. It matches on format alone, covering nine of the eleven formats, so it absorbed every assertion failure in those scenarios on every platform and reported each as an expected failure caused by a subprocess SIGPIPE that may not have occurred. AssertionError is the type every assertion in the suite raises, so this made the affected tests unable to fail: deleting the processor identity segment from cache_key produced eight expected failures and exit code 0. Ten genuinely failing concurrency tests were being reported the same way. A known-bug entry has to name the failure it actually knows about, so the entry now matches only the RuntimeError carrying the SIGPIPE signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cache_keyidentified the producing processor only by its version number. Two processors claiming the same(file, artifact_kind)pair at equalprocessor_versionderived an identical key and read back each other's artifacts as cache hits. Keys now carry the producing processor's identity as its own segment:Processor.processor_iddefaults per subclass to its own class name, so a processor that declares nothing still derives distinct keys — forgetting is safe rather than silently aliasing. The three shipped processors pin explicit identities (tabix-interval,bam-index,passthrough) so the values survive a class rename.ProcessorRegistry.registerrejects a duplicate identity, which is the only guard against two distinct classes pinning the same string.Changing the key invalidates every cached artifact by construction, so the migration sweep ships alongside it.
cfdb purge-legacy-cacheclears entries under the retired scheme from either backend, dry-run by default. That sweep subsumes the orphaned.bedpeandbigInteractartifacts stranded by #108 — they are retired-scheme keys too.Writing the tests surfaced a defect in the sweep itself.
is_legacy_cache_keychecked segment count and leaf shape but not the artifact-kind segment, sopurge_s3 --applywith an over-specified--s3-prefixdeleted live artifacts: seedingdev/encode/ENCFF1/index/tabix-interval/<md5>-v2and sweeping withprefix="dev/encode"returneddeleted=1and emptied the bucket. Since--s3-prefixreads fromWORKFLOW_S3_PREFIX, one environment-variable typo destroyed a live cache. Requiring a real artifact kind in that segment closes every mis-strip — removing one segment too many leaves the processor identity where the kind must be, and removing two leaves too few segments to match.The deploy shipping this starts against a fully cold cache.
Closes #109
Proposed changes
Fold a processor identity into the cache key
Add
processor_idtoProcessor, defaulted by__init_subclass__to the subclass's own class name. Read the default fromcls.__dict__rather than the MRO: a subclass may emit different bytes at the same version, so inheriting the parent's identity would recreate the aliasing the segment prevents. A value supplied by a mixin is discarded for the same reason.Validate a declared identity when the class is declared rather than at first use, so a malformed value fails on import instead of surfacing per-request inside a worker. Beyond the separator and null-byte guards
normalize_local_idalready applies, reject.and..— caught today only by the cache backend, a different module with a different rationale — and reject a value equal to an artifact kind.Reject two processors that share an identity
ProcessorRegistry.registerraises when another registered processor already claims the identity. The class-name default cannot catch two distinct classes pinning the same explicit string, which is the realistic collision. The guard keys on identity alone, so two processors claiming one format remain legal and order-resolved.Add the migration sweep
cfdb.workflows.purgeprovidespurge_s3andpurge_local;cfdb purge-legacy-cacheexposes both through the installed console script so the sweep runs from a deployed image. Exactly one store is purged per run — the command refuses when both an S3 bucket and a local root resolve rather than guessing.Two failure modes are surfaced rather than absorbed.
DeleteObjectsreports per-key failures in the response body instead of raising, so a sweep that deleted nothing for want of ans3:DeleteObjectgrant would report success. A response confirming fewer keys than requested means something was neither deleted nor complained about. Both raise, and the sweep is idempotent.The local sweep prunes only the directories its own deletions emptied. The root is operator-supplied, so an unrelated empty directory under it is not the sweep's to reclaim.
Correct stale test fixtures
tests/test_workflows/test_executor.pyandtests/integration/routines.pyheld four-segment key literals under a comment claiming they matched production output. Both now derive their keys. The executor stub's keys reach realJobRecord.artifact_cache_keysin integration runs.Several router cache-hit tests rebuilt the expected key with the same expression
cache_key_forevaluates, so they would have passed with the identity segment removed entirely. They now seed through the processor's own derivation.Test cases
TestCacheKeycache_keyis calledTestCacheKeycache_keyis called for eachTestCacheKeycache_keyis called for eachTestCacheKeycache_keyis called twiceTestIsLegacyCacheKeylogs/2024/01/{md5}-v2andencode/X/wat/{md5}-v2is_legacy_cache_keyis calledFalsefor bothTestIsLegacyCacheKeyis_legacy_cache_keyis calledFalseTestIsLegacyCacheKeyis_legacy_cache_keyis called on eachFalsefor allTestIsLegacyCacheKeycache_key(Hypothesis)is_legacy_cache_keyis calledFalseTestIsLegacyCacheKeyis_legacy_cache_keyis calledTrueTestNormalizeProcessorId.or..normalize_processor_idis calledValueErrorTestNormalizeProcessorIdnormalize_processor_idis calledValueErrorTestProcessorprocessor_id = ""TestProcessorValueErrorat definitionTestProcessorTestProcessorTestProcessorcache_key_foron eachTestProcessorRegistryregisteron the secondTestProcessorRegistrysupported_formatsregisteris calledTestProcessorRegistrylookup_foris calledTestDefaultRegistryapi/main.pyperformsregisterTestTabixIntervalProcessorprocessor_idis readtabix-intervalTestTabixIntervalProcessorcache_key_foron eachTestServeWorkflowArtifactOrDispatchTestServeWorkflowArtifactOrDispatchTestServeWorkflowArtifactOrDispatchTestProbeWorkflowReadinessFalse/statusagrees with the dispatch pathTestPurgeS3prefix="dev/encode"purge_s3runs appliedTestPurgeS3purge_s3runs appliedTestPurgeS3delete_objectsreports errorspurge_s3runs appliedRuntimeErrorTestPurgeS3purge_s3runs appliedRuntimeErrorTestPurgeS3dev/,dev-staging/,devops/purge_s3runs withprefix="dev"dev/entryTestPurgeLocalpurge_localruns appliedTestPurgeLocalpurge_localruns appliedkeysandpurgeTestPurgeLegacyCacheCommand--applyTestPurgeLegacyCacheCommandWORKFLOW_S3_BUCKETset alongside--local-rootTestPurgeLegacyCacheCommandTestLifespanProcessorRegistrydefault_registryunpatchedMagicMockhole in the existing lifespan testTestWoolExecutorcache_key_for1219 unit tests and 76 integration tests pass. The suite was mutation-checked rather than trusted green: removing the identity segment fails 26 tests including all five router-seam tests, and removing the artifact-kind guard fails exactly the five data-loss regression tests.
Deploy notes
Run
cfdb purge-legacy-cacheagainst each environment after this lands. Pass--s3-prefixexactly asWORKFLOW_S3_PREFIXis set for that environment; a prefix carrying extra segments is rejected by the artifact-kind check rather than acted on, and a prefix that is too short matches nothing.