fix(#311): don't apply DownloadResult.resource on rehydrate (cross-resource cache leak)#313
Merged
Merged
Conversation
…source cache leak) Closes #311. When the same file bytes are uploaded as two different CKAN resources, the second flow's ``database_task`` crashed with:: Postgres COPY failed: relation "<new_resource_id>" does not exist Root cause: ``analyze_task`` / ``validate_task`` / ``format_convert_task`` cache by file content (``dpp:{task_name}:{file_hash}``), so the second flow cache-hits and returns the cached result chain from the first flow — which still points all the way back to the first flow's ``DownloadResult`` carrying the first flow's full resource dict (with the OLD ``id``). ``database_task`` runs fresh (it doesn't cache — mutating stage). But ``_stage_run`` -> ``rehydrate`` walks the cached chain, and the ``DownloadResult`` branch in ``_apply_result`` did ``ctx.resource = dict(result.resource)`` — wholesale replacing the current flow's resource dict (set by ``_build_runtime_context`` from CKAN) with the cached one. From that point: * ``DatabaseStage._create_datastore_table`` uses ``context.resource["id"]`` (the OLD resource_id, from the cached resource dict) for ``datastore_create``. No-op — the table already exists from the first run. * ``DatabaseStage._copy_data`` uses ``context.resource_id`` (set fresh by ``_build_runtime_context``, NOT touched by rehydrate) for TRUNCATE. The NEW resource_id's table doesn't exist → TRUNCATE fails with ``UndefinedTable``. The fix: remove ``ctx.resource = dict(result.resource)`` from the ``DownloadResult`` branch of ``_apply_result``. Resource identity is per-flow-run state, not per-file-content state — ``_build_runtime_context`` already fetched the current flow's resource from CKAN, and that's the source of truth. What still works correctly: * ``ctx.file_hash`` / ``ctx.resource_url`` / ``ctx.content_length`` / ``ctx.tmp`` ARE still applied — those are genuinely content-derived and need to flow through the cached chain. * The download stage's only mutation of ``ctx.resource`` — ``["hash"] = file_hash`` (download.py:129) — happens for the current resource dict when the body runs. On a download_task cache hit (same resource_id), the body is skipped, but the PR #308 / #310 fix in MetadataStage already restores ``ctx.resource["hash"]`` from ``ctx.file_hash`` after its own re-fetch. Tests in new ``tests/test_rehydrate_resource_identity.py``: * ``test_rehydrate_does_not_overwrite_ctx_resource`` — the central #311 contract. * ``test_rehydrate_applies_content_fields_from_download_result`` — proves the fix didn't break what the cache IS supposed to propagate. * ``test_rehydrate_through_multi_layer_chain_preserves_ctx_resource`` — the realistic bug case (AnalyzeResult → ValidateResult → ConvertResult → DownloadResult chain). * ``test_file_hash_propagates_via_ctx_file_hash_not_via_resource_dict`` — pins the route the hash now takes to land on the persisted resource. Also updated existing ``test_rehydrate_reconstitutes_context_from_nested_results`` in ``test_prefect_flow.py`` to assert the new contract (current resource preserved, NOT replaced by cached) and to document why. End-to-end verification on the integration stack: uploaded the same 8-row CSV as two different resources back-to-back. First flow took 18s (full pipeline), second flow took 10s (cache hits on validate / analyze, no crash), both ended with ``datastore_active=True`` and queryable datastore data with correct rows. Pre-fix, the second flow crashed with "relation does not exist". Tests: 213/213 unit tests pass on dpp-test (was 209; +4 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a cross-resource cache leak during Prefect result-chain rehydration by ensuring cached DownloadResult.resource does not overwrite the current flow run’s CKAN resource identity (preventing wrong resource_id from propagating into database_task).
Changes:
- Stop applying
DownloadResult.resourceontoctx.resourceduringrehydrate, preserving per-flow-run resource identity. - Update existing rehydration unit test to assert the new “resource identity is not overwritten” contract.
- Add a focused regression test suite covering the #311 contract and confirming content-derived fields still propagate through nested cached result chains.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ckanext/datapusher_plus/jobs/runtime_context.py |
Prevents rehydrate from clobbering ctx.resource from cached DownloadResult across flows. |
tests/test_prefect_flow.py |
Updates rehydration test expectations to reflect the new resource-identity contract. |
tests/test_rehydrate_resource_identity.py |
Adds regression coverage for cross-resource cache-hit rehydration behavior (#311). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tity (Copilot #313) Copilot flagged ``os``, ``Any``, ``Dict``, ``List`` as imported but never used. ``grep -nE '\b(os|Any|Dict|List)\b'`` confirmed — the imports only appear on the import lines themselves. Removed both lines. 4/4 regression tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
Closes #311.
When the same file bytes are uploaded as two different CKAN resources, the second flow's
database_taskcrashed with:Caught during PR #309's post-merge smoke test, filed as #311, now fixed and end-to-end verified.
Root cause
analyze_task/validate_task/format_convert_taskcache by file content (dpp:{task_name}:{file_hash}), so the second flow cache-hits and returns the first flow's cached result chain — which still points all the way back to the first flow'sDownloadResultcarrying the first flow's full resource dict (with the OLDid).database_taskruns fresh (mutating stage, no cache). But_stage_run→rehydratewalks the cached chain, and theDownloadResultbranch in_apply_resultdidctx.resource = dict(result.resource)— wholesale replacing the current flow's resource dict (set by_build_runtime_contextfrom CKAN) with the cached one. From that point:DatabaseStage._create_datastore_tableusedcontext.resource["id"](OLD resource_id, from the cached dict) fordatastore_create— no-op, the table already exists from the first run.DatabaseStage._copy_datausedcontext.resource_id(set fresh by_build_runtime_context, NOT touched by rehydrate) for TRUNCATE — the NEW resource_id's table doesn't exist, TRUNCATE fails withUndefinedTable.Fix
Remove
ctx.resource = dict(result.resource)from theDownloadResultbranch of_apply_result. Resource identity is per-flow-run state, not per-file-content state —_build_runtime_contextalready fetched the current flow's resource from CKAN, and that's the source of truth.What still works correctly:
ctx.file_hash/ctx.resource_url/ctx.content_length/ctx.tmpARE still applied — those are genuinely content-derived.ctx.resource(["hash"] = file_hash,download.py:129) happens for the current resource dict when the body runs. On a download_task cache hit (same resource_id), the body is skipped, but the metadata stage drops file hash on re-fetch — persisted resource hash is always empty #310 fix in MetadataStage already restoresctx.resource["hash"]fromctx.file_hashafter its own re-fetch.Test plan
dpp-test(was 209; +4 new + 1 existing updated for the new contract).tests/test_rehydrate_resource_identity.pyregression suite:ctx.resourcenot overwritten).AnalyzeResult.upstream).file_hashpropagation route viactx.file_hash(not the resource dict).test_rehydrate_reconstitutes_context_from_nested_resultsupdated to assert the new contract with documentation explaining why.datastore_active=True, identical BLAKE3 hash, and queryable datastore data with correct rows. Pre-fix, the second flow crashed withrelation does not exist.🤖 Generated with Claude Code