Skip to content

Raise TaskResourceNotFound instead of bare DoesNotExist in tasks - #2439

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

Raise TaskResourceNotFound instead of bare DoesNotExist in tasks#2439
wussh wants to merge 2 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.

@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 Outdated
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.

wussh added a commit to wussh/pulp_container that referenced this pull request Jul 31, 2026
- 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.

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.

Add TaskResourceNotFound(PulpException) and wrap each top-of-task
object lookup in an explicit try/except, mirroring how the digest-fetch
build errors are handled rather than introducing a generic helper (per
review feedback: a generic get_task_resource_or_raise wrapper was
rejected in favor of explicit per-call-site handling).

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.
@wussh

wussh commented Aug 6, 2026

Copy link
Copy Markdown
Author

@gerrod3 @jobselko rebased this branch directly onto `main` and re-did the fix as a single commit (517637e), which resolves both open items:

Force-pushed the rebased branch just now — CI is green apart from the known flaky azure job and an infra-only podman config error on lowerbounds, both unrelated to this diff.

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