feat: add upload breadcrumbs to test results tasks#716
feat: add upload breadcrumbs to test results tasks#716thomasrockhu-codecov wants to merge 1 commit intomainfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| commit_sha=commitid, | ||
| repo_id=repoid, | ||
| milestone=Milestones.NOTIFICATIONS_SENT, | ||
| ) |
There was a problem hiding this comment.
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.
| commit_sha=commitid, | ||
| repo_id=repoid, | ||
| error=Errors.INTERNAL_LOCK_ERROR, | ||
| ) |
There was a problem hiding this comment.
Error breadcrumb missing milestone, inconsistent with all other callsites
Low Severity
The _call_upload_breadcrumb_task call for the lock error case passes only error=Errors.INTERNAL_LOCK_ERROR without a milestone. Every other error breadcrumb in the codebase — preprocess_upload.py, notify.py, and the API helpers — always pairs an error with a milestone so the breadcrumb indicates which pipeline stage the error occurred at. The omission means this breadcrumb will have milestone=None, making it harder to correlate with the stage (NOTIFICATIONS_TRIGGERED) that was already recorded.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #716 +/- ##
==========================================
- Coverage 92.25% 92.24% -0.02%
==========================================
Files 1302 1302
Lines 47837 47891 +54
Branches 1628 1628
==========================================
+ Hits 44131 44176 +45
- Misses 3397 3406 +9
Partials 309 309
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Add lock lifecycle breadcrumbs (LOCK_ACQUIRING, LOCK_ACQUIRED, LOCK_RELEASED) and processing/notification breadcrumbs to TestResultsProcessorTask and TestResultsFinisherTask. Each breadcrumb includes task_name and parent_task_id for pipeline traceability. Processor breadcrumbs: - PROCESSING_UPLOAD at start of processing - UPLOAD_COMPLETE on success - UNKNOWN error on unhandled exception Finisher breadcrumbs: - NOTIFICATIONS_TRIGGERED at start - Lock lifecycle (acquiring/acquired/released/error) - NOTIFICATIONS_SENT on completion Co-authored-by: Cursor <cursoragent@cursor.com>
thomasrockhu-codecov
force-pushed
the
tomhu/tr-upload-breadcrumbs
branch
from
c70ecdb to
6354807
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| "repo_id": repoid, | ||
| "task_name": self.name, | ||
| "parent_task_id": self.request.parent_id, | ||
| } |
There was a problem hiding this comment.
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)
|
|
||
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.LOCK_ACQUIRING, **bc_kwargs | ||
| ) |
There was a problem hiding this comment.
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)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| "repo_id": repoid, | ||
| "upload_ids": upload_ids, |
There was a problem hiding this comment.
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.
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.NOTIFICATIONS_SENT, **bc_kwargs | ||
| ) |
There was a problem hiding this comment.
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.
1 participant
Summary
_call_upload_breadcrumb_taskcalls toTestResultsProcessorTaskandTestResultsFinisherTaskPROCESSING_UPLOAD,UPLOAD_COMPLETE,NOTIFICATIONS_TRIGGERED,NOTIFICATIONS_SENT) and errors (UNKNOWN,INTERNAL_LOCK_ERROR) at each stage of the test results pipelineTest plan
Made with Cursor
Note
Low Risk
Primarily adds fire-and-forget telemetry calls around existing task logic; functional behavior should be unchanged aside from slight extra task enqueueing and potential noise if breadcrumbs misfire.
Overview
Adds upload breadcrumb tracking to the test results pipeline by instrumenting
TestResultsProcessorTaskandTestResultsFinisherTaskwith_call_upload_breadcrumb_taskcalls.The processor now records
PROCESSING_UPLOADandUPLOAD_COMPLETEmilestones (includingupload_ids) and emits anUNKNOWNerror breadcrumb on unhandled exceptions. The finisher now records notification/lock milestones (NOTIFICATIONS_TRIGGERED,LOCK_ACQUIRING/ACQUIRED/RELEASED,NOTIFICATIONS_SENT) and logs anINTERNAL_LOCK_ERRORbreadcrumb when lock acquisition retries occur/fail.Written by Cursor Bugbot for commit 6354807. This will update automatically on new commits. Configure here.