Skip to content

Raise TaskResourceNotFound instead of bare DoesNotExist in tasks - #2439

Open
wussh wants to merge 5 commits into
pulp:mainfrom
wussh:harden-task-object-lookups
Open

Raise TaskResourceNotFound instead of bare DoesNotExist in tasks#2439
wussh wants to merge 5 commits into
pulp:mainfrom
wussh:harden-task-object-lookups

Conversation

@wussh

@wussh wussh commented Jul 27, 2026

Copy link
Copy Markdown

Summary

  • pulpcore's task executor treats any task exception that isn't a PulpException subclass as deprecated: it logs a pulpcore.deprecation warning today and will sanitize the message away entirely in pulpcore 3.130.
  • Every task that looks up its arguments' referenced objects by pk (repository, remote, manifest, signing service, artifact, ...) raises a bare Django DoesNotExist when that object was deleted between dispatch and execution. This is non-deterministic — a different one surfaces on each CI run depending on test/teardown timing (observed both ManifestSigningService and ContainerDistribution DoesNotExist warnings on unrelated PRs' CI, e.g. Fix tag-based manifest pulls 404ing while digest-based pulls succeed #2438).
  • Adds TaskResourceNotFound(PulpException) and a get_task_resource_or_raise() helper (app/utils.py), used at the top-of-task object lookups in tag.py, untag.py, recursive_add.py, recursive_remove.py, synchronize.py, download_image_data.py, sign.py, and builder.py.
  • Left the intentional try/except DoesNotExist in builder.py's get_or_create_blob alone — that's expected control flow (create-on-miss), not an unhandled lookup failure.
  • Also pulled in two related, independently-discovered deprecations-noise fixes so this PR is self-contained (same root cause class — non-PulpException raised in a task — found while chasing Domain-scoped container registry: tag-based manifest fetch 404s, digest-based works (pulpcore 3.113.0 / pulp_container 2.28.0) #2417's CI): builder.py's podman build/push failure now raises ContainerBuildError(PulpException) instead of bare Exception, and test_domains.py's cdomain_factory fixture now cleans up ContainerNamespace (a PROTECT FK to Domain) before teardown deletes the domain, instead of leaving it to raise ProtectedError.

Not addressed (out of scope for this PR): pulpcore's own generic tasks (e.g. general_multi_delete, which can raise ContainerDistribution.DoesNotExist) are outside pulp_container's control, so the deprecations CI job may still show occasional pulpcore-origin warnings even after this merges. This is a noise-reduction / forward-compat cleanup for pulp_container's own task code, not a guarantee of a fully green deprecations job.

Test plan

  • ruff check / ruff format --check clean on all touched files.
  • Verified by grep that no other unhandled Model.objects.get(pk=...) task-entry lookups were missed in app/tasks/.
  • CI (lint/build/test/docs/sanity/ready-to-ship) — all green; deprecations job re-checked, remaining warnings confirmed unrelated to this PR's changes.

pulpcore's task executor treats any task exception that isn't a
PulpException subclass as deprecated: it logs a pulpcore.deprecation
warning today and will sanitize the message away entirely in pulpcore
3.130. Every task that looks up its arguments' referenced objects by
pk (repository, remote, manifest, signing service, artifact, ...) hit
this whenever that object was deleted between dispatch and execution
— non-deterministic, so a different one surfaces on each CI run
(observed both ManifestSigningService and ContainerDistribution
DoesNotExist warnings on unrelated PRs).

Add TaskResourceNotFound(PulpException) and a get_task_resource_or_raise()
helper in app/utils.py, and use it at the top-of-task object lookups in
tag.py, untag.py, recursive_add.py, recursive_remove.py, synchronize.py,
download_image_data.py, sign.py, and builder.py. Leaves the intentional
try/except DoesNotExist in builder.py's get_or_create_blob alone, since
that's expected control flow, not an unhandled lookup failure.

Note: pulpcore's own generic tasks (e.g. general_multi_delete, which can
raise ContainerDistribution.DoesNotExist) are outside pulp_container's
control and are not addressed by this change.
wussh added 2 commits July 27, 2026 16:59
pulpcore treats any task exception that isn't a PulpException subclass
as deprecated: it logs a pulpcore.deprecation warning today and will
sanitize the error message away entirely in pulpcore 3.130, losing the
actual podman/buildah stderr. build_image_from_scratch and the
deprecated build_image_from_containerfile both raised bare Exception
on a failed podman build/push, tripping this path — surfaced as CI
noise in the "deprecations" check, but a real forward-compat bug.

Add ContainerBuildError(PulpException) and raise it with the decoded
stderr instead; pytest.raises(PulpTaskError) assertions in
test_build_images.py are unaffected since the exception still
propagates through task.error the same way.
ContainerNamespace has a PROTECT foreign key to Domain. cdomain_factory's
teardown cleared content-redirect guards but never ContainerNamespace
objects, so any domain-scoped ContainerDistribution left one behind
(auto-created by ContainerNamespaceSerializer.get_or_create). When
pulpcore's own domain_factory teardown then deleted the domain, Django
raised ProtectedError — sanitized into a pulpcore.deprecation warning
that failed the "deprecations" CI check.

test_pull_in_domain creates two distributions and cleaned up neither
namespace, making it the concrete trigger. Query-and-delete namespaces
by domain in cdomain_factory's teardown instead of relying on each test
to remember; safe to run even for tests that already clean their own
namespace via add_to_cleanup since it re-lists current state.

@gerrod3 gerrod3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sold on the retrieve or raise logic/exception. The other builder exception looks good.

Comment thread CHANGES/+container-build-error-sanitization.bugfix Outdated
Comment thread pulp_container/tests/functional/api/test_domains.py
Comment thread pulp_container/app/exceptions.py Outdated
@jobselko

jobselko commented Jul 28, 2026

Copy link
Copy Markdown
Member

@wussh I see this PR duplicates some code from #2438. It would be helpful to put those changes in a separate commit and clearly mark them to make the PR easier to review.

- error_code: use the CON00XX naming convention (CON0001, CON0002)
  jobselko and gerrod3 agreed on, instead of the ad-hoc
  PLP_CONTAINER_* strings.
- CHANGES fragments: use single backticks, since they render as
  markdown.
- cdomain_factory: adopt gerrod3's version of the namespace cleanup
  from pulp#2424 (suppress errors per-namespace/guard instead of letting
  one failure abort the rest, and wait on the delete task via
  monitor_task).
@wussh

wussh commented Jul 31, 2026

Copy link
Copy Markdown
Author

Addressed:

@jobselko the duplicated ContainerBuildError/cdomain_factory changes from #2438 are already in their own separate commits (Raise ContainerBuildError instead of bare Exception in image builder, Clean up ContainerNamespace before domain deletion in domain tests) — let me know if you'd like them called out any more clearly than that.

@gerrod3 re: "not sold on the retrieve or raise logic/exception" — could you elaborate on what you'd prefer instead? Happy to rework get_task_resource_or_raise once I understand the concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants