Skip to content

Fix/database deletes #767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jason-ford-codecov wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix/database deletes #767

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
71b8cae
adding comment
jason-ford-codecov Mar 11, 2026
3d2ed6b
updated comment
jason-ford-codecov Mar 11, 2026
b04fd1a
fixing comment
jason-ford-codecov Mar 11, 2026
6115081
fixing lint
jason-ford-codecov Mar 11, 2026
4a3abdf
adding logging
jason-ford-codecov Mar 12, 2026
8a8170b
adjusting django results in logs
jason-ford-codecov Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion apps/worker/services/cleanup/cleanup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import logging

from celery.exceptions import SoftTimeLimitExceeded
from django.core.exceptions import EmptyResultSet
from django.db import OperationalError
from django.db.models.query import QuerySet

from services.cleanup.models import MANUAL_CLEANUP
Expand Down Expand Up @@ -31,5 +34,34 @@ def cleanup_queryset(query: QuerySet, context: CleanupContext):
if manual_cleanup is not None:
manual_cleanup(context, query)
else:
cleaned_models = query._raw_delete(query.db)
try:
query_str = str(query.query)[:500]
except EmptyResultSet:
query_str = "(empty)"
log.info(
"cleanup_queryset: deleting %s with query %s",
model.__name__,
query_str,
)
try:
cleaned_models = query._raw_delete(query.db)
except SoftTimeLimitExceeded:
log.warning(
"cleanup_queryset: soft time limit hit, rolling back delete of %s",
model.__name__,
exc_info=True,
)
raise
Comment on lines +46 to +54
Copy link
Contributor

@sentry sentry bot Mar 12, 2026

Choose a reason for hiding this comment

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

Bug: The _raw_delete call can raise EmptyResultSet for empty querysets, but this exception is not caught, which will cause the cleanup task to fail unnecessarily.
Severity: MEDIUM

Suggested Fix

Wrap the _raw_delete(query) call in a try...except EmptyResultSet block. Inside the except block, you can simply pass or log that no rows were deleted, as this is an expected outcome for an empty queryset.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: apps/worker/services/cleanup/cleanup.py#L46-L54

Potential issue: The `_raw_delete` function in `cleanup_queryset` is not wrapped in a
`try...except` block that handles `django.db.models.query.EmptyResultSet`. This
exception can be raised when processing an empty queryset, which is a normal scenario
when there are no records to delete. While the code correctly handles this exception
when generating a query string for logging, it fails to do so for the actual delete
operation. This inconsistency will cause the entire cleanup task to fail when it
encounters an empty set of records, treating a valid "0 rows deleted" case as an error.

except OperationalError:
log.warning(
"cleanup_queryset: db connection dropped, rolling back delete of %s",
model.__name__,
exc_info=True,
)
raise
log.info(
"cleanup_queryset: deleted %d rows from %s",
cleaned_models,
model.__name__,
)
context.add_progress(cleaned_models)
Loading