Skip to content

feat: add upload breadcrumbs to test results tasks #716

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
thomasrockhu-codecov wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add upload breadcrumbs to test results tasks #716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 30 additions & 0 deletions apps/worker/tasks/test_results_finisher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from services.lock_manager import LockManager, LockRetry, LockType
from services.test_analytics.ta_finish_upload import ta_finish_upload
from shared.celery_config import test_results_finisher_task_name
from shared.django_apps.upload_breadcrumbs.models import Errors, Milestones
from shared.yaml import UserYaml
from tasks.base import BaseCodecovTask
from tasks.notify import notify_task_name
Expand All @@ -35,6 +36,17 @@ def run_impl(
}
log.info("Starting test results finisher task", extra=self.extra_dict)

bc_kwargs = {
"commit_sha": commitid,
"repo_id": repoid,
"task_name": self.name,
"parent_task_id": self.request.parent_id,
}
Copy link

@cursor cursor bot Feb 19, 2026

Choose a reason for hiding this comment

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

Extra kwargs crash breadcrumb calls and break tasks

High Severity

The bc_kwargs dicts include task_name and parent_task_id, but _call_upload_breadcrumb_task only accepts commit_sha, repo_id, milestone, upload_ids, error, and error_text — it does not accept **kwargs. When **bc_kwargs is unpacked at each call site, Python raises a TypeError for the unexpected keyword arguments. This error occurs at the call site, not inside the method body, so the method's internal try/except cannot catch it. Since the breadcrumb calls are meant to be fire-and-forget, this will instead crash the entire task.

Additional Locations (1)

Fix in Cursor Fix in Web


self._call_upload_breadcrumb_task(
milestone=Milestones.NOTIFICATIONS_TRIGGERED, **bc_kwargs
)

lock_manager = LockManager(
repoid=repoid,
commitid=commitid,
Expand All @@ -43,6 +55,10 @@ def run_impl(
blocking_timeout=None,
)

self._call_upload_breadcrumb_task(
milestone=Milestones.LOCK_ACQUIRING, **bc_kwargs
)
Copy link

@cursor cursor bot Feb 19, 2026

Choose a reason for hiding this comment

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

Nonexistent milestone enum values cause AttributeError

High Severity

Milestones.LOCK_ACQUIRING, Milestones.LOCK_ACQUIRED, and Milestones.LOCK_RELEASED don't exist in the Milestones enum (which only defines FETCHING_COMMIT_DETAILS, COMMIT_PROCESSED, PREPARING_FOR_REPORT, READY_FOR_REPORT, WAITING_FOR_COVERAGE_UPLOAD, COMPILING_UPLOADS, PROCESSING_UPLOAD, NOTIFICATIONS_TRIGGERED, UPLOAD_COMPLETE, NOTIFICATIONS_SENT). Accessing these attributes will raise AttributeError at runtime, crashing the finisher task.

Additional Locations (2)

Fix in Cursor Fix in Web


try:
# this needs to be the coverage notification lock
# since both tests post/edit the same comment
Expand All @@ -51,13 +67,20 @@ def run_impl(
max_retries=5,
retry_num=self.attempts,
):
self._call_upload_breadcrumb_task(
milestone=Milestones.LOCK_ACQUIRED, **bc_kwargs
)
finisher_result = self.process_impl_within_lock(
db_session=db_session,
repoid=repoid,
commitid=commitid,
commit_yaml=UserYaml.from_dict(commit_yaml),
**kwargs,
)
self._call_upload_breadcrumb_task(
milestone=Milestones.LOCK_RELEASED, **bc_kwargs
)

if finisher_result["queue_notify"]:
self.app.tasks[notify_task_name].apply_async(
args=None,
Expand All @@ -68,9 +91,16 @@ def run_impl(
},
)

self._call_upload_breadcrumb_task(
milestone=Milestones.NOTIFICATIONS_SENT, **bc_kwargs
)
Copy link

@cursor cursor bot Feb 19, 2026

Choose a reason for hiding this comment

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

NOTIFICATIONS_SENT breadcrumb recorded when no notification sent

Medium Severity

The Milestones.NOTIFICATIONS_SENT breadcrumb is recorded unconditionally after the lock block, but the notification is only actually queued when finisher_result["queue_notify"] is True. When queue_notify is False, no notification is sent yet the breadcrumb still claims NOTIFICATIONS_SENT, producing misleading telemetry data. The breadcrumb call needs to be inside the if finisher_result["queue_notify"]: block.

Fix in Cursor Fix in Web

Comment on lines +94 to +96
Copy link

@sentry sentry bot Feb 19, 2026

Choose a reason for hiding this comment

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

Bug: The NOTIFICATIONS_SENT breadcrumb is recorded even when no notification task is actually scheduled, leading to incorrect milestone tracking.
Severity: MEDIUM

Suggested Fix

Move the call to _call_upload_breadcrumb_task for the NOTIFICATIONS_SENT milestone inside the if finisher_result["queue_notify"]: block. This ensures the breadcrumb is only recorded when a notification task is actually queued.

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/tasks/test_results_finisher.py#L94-L96

Potential issue: In `test_results_finisher.py`, the `NOTIFICATIONS_SENT` milestone
breadcrumb is recorded unconditionally after attempting to queue a notification task.
However, the notification task is only queued if `finisher_result["queue_notify"]` is
true. In cases where it is false (e.g., comments are disabled or no pull request is
found), no notification is sent, but the breadcrumb is still recorded, creating
misleading monitoring data. This can hamper debugging efforts by incorrectly indicating
that notifications were sent.

Did we get this right? 👍 / 👎 to inform future reviews.


return finisher_result

except LockRetry as retry:
self._call_upload_breadcrumb_task(
error=Errors.INTERNAL_LOCK_ERROR, **bc_kwargs
)
if retry.max_retries_exceeded:
log.error(
"Not retrying lock acquisition - max retries exceeded",
Expand Down
42 changes: 35 additions & 7 deletions apps/worker/tasks/test_results_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from services.processing.types import UploadArguments
from services.test_analytics.ta_processor import ta_processor
from shared.celery_config import test_results_processor_task_name
from shared.django_apps.upload_breadcrumbs.models import Errors, Milestones
from tasks.base import BaseCodecovTask

log = logging.getLogger(__name__)
Expand All @@ -25,14 +26,41 @@ def run_impl(
arguments_list: list[UploadArguments],
**kwargs,
) -> bool:
for argument in arguments_list:
ta_processor(
repoid=repoid,
commitid=commitid,
commit_yaml=commit_yaml,
argument=argument,
update_state=True,
upload_ids = [arg["upload_id"] for arg in arguments_list if "upload_id" in arg]

bc_kwargs = {
"commit_sha": commitid,
"repo_id": repoid,
"upload_ids": upload_ids,
Comment on lines +33 to +34
Copy link

@sentry sentry bot Feb 19, 2026

Choose a reason for hiding this comment

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

Bug: The call to _call_upload_breadcrumb_task includes unsupported keyword arguments task_name and parent_task_id, which will cause a TypeError at runtime.
Severity: CRITICAL

Suggested Fix

Remove the task_name and parent_task_id keys from the bc_kwargs dictionary in both test_results_processor.py and test_results_finisher.py before passing it to _call_upload_breadcrumb_task.

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/tasks/test_results_processor.py#L33-L34

Potential issue: The `_call_upload_breadcrumb_task` method is called with extra keyword
arguments, `task_name` and `parent_task_id`, which are not part of its defined
signature. The method does not accept arbitrary keyword arguments (`**kwargs`), so
passing these unsupported parameters will cause the application to raise a `TypeError`
at runtime. This issue occurs in both `test_results_processor.py` and
`test_results_finisher.py` whenever a breadcrumb is recorded, causing the tasks to fail.

Did we get this right? 👍 / 👎 to inform future reviews.

"task_name": self.name,
"parent_task_id": self.request.parent_id,
}

self._call_upload_breadcrumb_task(
milestone=Milestones.PROCESSING_UPLOAD, **bc_kwargs
)

try:
for argument in arguments_list:
ta_processor(
repoid=repoid,
commitid=commitid,
commit_yaml=commit_yaml,
argument=argument,
update_state=True,
)
except Exception:
self._call_upload_breadcrumb_task(
error=Errors.UNKNOWN,
error_text="Unhandled exception during test results processing",
**bc_kwargs,
)
raise

self._call_upload_breadcrumb_task(
milestone=Milestones.UPLOAD_COMPLETE, **bc_kwargs
)

return True


Expand Down
Loading