Update delete task to skip over already deleted objects - #7930
Conversation
fixes: pulp#7910 Generated by: cursor-grok-4.5
gerrod3
left a comment
There was a problem hiding this comment.
This only covers the case where the user dispatches multiple delete tasks on the same object. Every N+1 task will no result in a no-op instead of error. However, there are still many scenarios where the delete can fail that isn't caught with this change. Here are some AI thought of:
Business-rule / hook failures
RepositoryVersion: last complete version → APIException
RepositoryVersion: protected version → BEFORE_DELETE raises
RepositoryVersion: squash didn’t clear memberships → RuntimeError
Other BEFORE_DELETE hooks (domains, remotes, etc.)Integrity / relation failures
ProtectedError / RestrictedError when related rows block the delete (PROTECT/RESTRICT), e.g. ContentArtifact → Artifact, AlternateContentSource → Remote, RepositoryContent.version_added/removed → RepositoryVersion
Other DB integrity errors from cascade cleanupConcurrency / ordering
Object (or a later multi-delete target) removed between get/cast and delete
In general_multi_delete, deleting A cascades away B still in the list → later B.delete() can fail or delete nothing
Deadlocks / lock waits (notably RepositoryVersion’s select_for_update)Operational
DB timeouts, disconnects, OOM on large cascade trees
Nested work inside overrides (cache, bulk _raw_delete, etc.) raising unexpectedly
general_multi_delete specificallyOne failing delete() aborts the outer transaction.atomic(), so earlier deletes in that batch roll back.
Usually not task failures
Artifact file removal runs on transaction.on_commit, so storage errors there don’t fail the delete call itself.
| log.info( | ||
| "Skipping delete of %s pk=%s; it no longer exists.", | ||
| model.__name__, | ||
| instance_id, | ||
| ) |
There was a problem hiding this comment.
Some questions about logging:
- What is our current policy of using
gettext? Should we still be wrapping log statements inside it? - Does something like this deserved to be logged if we put it in the task output? I didn't tell the AI to remove the logs after adding the output, but my general question is what things are important enough that they need to be logged?
There was a problem hiding this comment.
Good questions. RE 1) - iirc, our decision is that "only things going back to an end-user need I18N", esp in combination with more-specific exception IDs making searching functional in the face of L10N. Logs are only seen by admins, and don't have specific-IDs to support searching, so we'll never localize them.
RE 2) - thinking out loud here - admins want/need debugging/problem-determination help, so "what happened when" is at the top of the should-we-log list. This specific output is in an odd place tho - we're telling the user what we did, but all the user really cares about is "is the thing I deleted gone", and in this case it is, even tho someone else deleted it.
In general, it's important to log "I decided I couldn't/wouldn't do X because of reason Y". Things like this one, which is "I went to do X and it already happened", honestly might be better for debug-level? I'm trying to think about what action we/an-admin might take upon seeing this message - since really things worked, even if there was a collision.
| for model_label, count in instance.delete()[1].items(): | ||
| counts[model_label] += count | ||
| output.update(counts) | ||
| return dict(output) |
There was a problem hiding this comment.
Task outputs can be anything as long as they are serializable, correct?
Also added delete results output to the tasks.
fixes: #7910
Generated by: cursor-grok-4.5
📜 Checklist
See: Pull Request Walkthrough