-
Notifications
You must be signed in to change notification settings - Fork 13
feat: add upload breadcrumbs to bundle analysis tasks #715
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.
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
base: tomhu/revert-pipeline-context
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| BUNDLE_ANALYSIS_PROCESSOR_MAX_RETRIES, | ||
| bundle_analysis_processor_task_name, | ||
| ) | ||
| from shared.django_apps.upload_breadcrumbs.models import Errors, Milestones | ||
| from shared.reports.enums import UploadState | ||
| from shared.yaml import UserYaml | ||
| from tasks.base import BaseCodecovTask | ||
|
|
@@ -124,21 +125,42 @@ def run_impl( | |
| commitid=commitid, | ||
| ) | ||
|
|
||
| bc_kwargs = { | ||
| "commit_sha": commitid, | ||
| "repo_id": repoid, | ||
| "task_name": self.name, | ||
| "parent_task_id": self.request.parent_id, | ||
| } | ||
|
|
||
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.LOCK_ACQUIRING, **bc_kwargs | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-existent milestone enum values cause AttributeErrorHigh Severity
Additional Locations (1) |
||
| ) | ||
|
|
||
| try: | ||
| with lock_manager.locked( | ||
| LockType.BUNDLE_ANALYSIS_PROCESSING, | ||
| retry_num=self.attempts, | ||
| max_retries=self.max_retries, | ||
| ): | ||
| return self.process_impl_within_lock( | ||
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.LOCK_ACQUIRED, **bc_kwargs | ||
| ) | ||
| result = self.process_impl_within_lock( | ||
| db_session, | ||
| repoid, | ||
| commitid, | ||
| UserYaml.from_dict(commit_yaml), | ||
| params, | ||
| previous_result, | ||
| ) | ||
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.LOCK_RELEASED, **bc_kwargs | ||
| ) | ||
| return result | ||
| except LockRetry as retry: | ||
| self._call_upload_breadcrumb_task( | ||
| error=Errors.INTERNAL_LOCK_ERROR, **bc_kwargs | ||
| ) | ||
| # Honor LockManager cap (Redis attempt count) so re-delivered messages stop. | ||
| if retry.max_retries_exceeded or self._has_exceeded_max_attempts( | ||
| self.max_retries | ||
|
|
@@ -255,6 +277,18 @@ def process_impl_within_lock( | |
|
|
||
| assert upload is not None | ||
|
|
||
| bc_kwargs = { | ||
| "commit_sha": commitid, | ||
| "repo_id": repoid, | ||
| "upload_ids": [upload.id_], | ||
| "task_name": self.name, | ||
| "parent_task_id": self.request.parent_id, | ||
| } | ||
|
|
||
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.PROCESSING_UPLOAD, **bc_kwargs | ||
| ) | ||
|
|
||
| # Override base commit of comparisons with a custom commit SHA if applicable | ||
| compare_sha: str | None = cast( | ||
| str | None, params.get("bundle_analysis_compare_sha") | ||
|
|
@@ -285,6 +319,9 @@ def process_impl_within_lock( | |
| attempts=self.attempts, | ||
| max_retries=self.max_retries, | ||
| ) | ||
| self._call_upload_breadcrumb_task( | ||
| error=Errors.INTERNAL_OUT_OF_RETRIES, **bc_kwargs | ||
| ) | ||
| try: | ||
| result.update_upload(carriedforward=carriedforward) | ||
| db_session.commit() | ||
|
|
@@ -301,6 +338,9 @@ def process_impl_within_lock( | |
| db_session, upload, commitid, repoid, log_suffix=" fallback" | ||
| ) | ||
| return processing_results | ||
| self._call_upload_breadcrumb_task( | ||
| error=Errors.INTERNAL_RETRYING, **bc_kwargs | ||
| ) | ||
| log.warn( | ||
| "Attempting to retry bundle analysis upload", | ||
| extra={ | ||
|
|
@@ -319,10 +359,12 @@ def process_impl_within_lock( | |
| db_session.commit() | ||
|
|
||
| processing_results.append(result.as_dict()) | ||
|
|
||
| self._call_upload_breadcrumb_task( | ||
| milestone=Milestones.UPLOAD_COMPLETE, **bc_kwargs | ||
| ) | ||
| except (CeleryError, SoftTimeLimitExceeded): | ||
| # This generally happens when the task needs to be retried because we attempt to access | ||
| # the upload before it is saved to GCS. Anecdotally, it takes around 30s for it to be | ||
| # saved, but if the BA processor runs before that we will error and need to retry. | ||
| self._call_upload_breadcrumb_task(error=Errors.TASK_TIMED_OUT, **bc_kwargs) | ||
| raise | ||
| except Exception: | ||
| log.exception( | ||
|
|
@@ -336,6 +378,11 @@ def process_impl_within_lock( | |
| "parent_task": self.request.parent_id, | ||
| }, | ||
| ) | ||
| self._call_upload_breadcrumb_task( | ||
| error=Errors.UNKNOWN, | ||
| error_text="Unhandled exception during bundle analysis processing", | ||
| **bc_kwargs, | ||
| ) | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| _set_upload_error_and_commit(db_session, upload, commitid, repoid) | ||
| return processing_results | ||
| finally: | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected keyword arguments crash all breadcrumb calls
High Severity
The
bc_kwargsdictionaries includetask_nameandparent_task_id, but_call_upload_breadcrumb_taskonly acceptscommit_sha,repo_id,milestone,upload_ids,error, anderror_text— it has no**kwargs. Every call using**bc_kwargswill raise aTypeErrorfor unexpected keyword arguments. Since the error occurs at the call site (not inside the method's try/except), it will propagate unhandled and crash the bundle analysis tasks.Additional Locations (2)
apps/worker/tasks/bundle_analysis_notify.py#L58-L64apps/worker/tasks/bundle_analysis_processor.py#L279-L286