Conversation
…n/add-exporter-tasks-to-celery-registry
…n/adjust-ownerid-task-params
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
==========================================
+ Coverage 92.51% 92.53% +0.02%
==========================================
Files 1295 1295
Lines 47689 47678 -11
Branches 1592 1592
==========================================
Hits 44121 44121
+ Misses 3259 3248 -11
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! |
| export_id=export_id, | ||
| since_date_iso=since_date_iso, | ||
| ), | ||
| ).on_error(error_callback), |
There was a problem hiding this comment.
out of my curioisity: why are we separating out the export_owner_cleanup task to be separate after each of these other tasks vs before?
There was a problem hiding this comment.
Basically the cleanup is a callback task that we'll use if any task fails, so if you have some files linger in memory or smth, the cleanup task will empty and clean up the container. I guess the idea is only to run it when it needs to be ran you know?
There was a problem hiding this comment.
This could create the possible scenario that the cleanup callback would be called multiple times at the same time though if each of the tasks fails even though it tries to do the same thing on the same objects. There's no difference in what the cleanup does depending on what task failed since if any of the tasks fail, we want everything to cleaned up and marked as not usable, right? If that's the case, I think the old version would work better
There was a problem hiding this comment.
Ah I see what you mean, ya that's a good insight. I'll address and have the whole chord throw an error if any of its sub-tasks has an error
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| "SQL export failed", | ||
| extra={"export_id": export_id, "error": str(e)}, | ||
| ) | ||
| _mark_export_failed_by_id(export_id, str(e)) |
There was a problem hiding this comment.
Removed error handling risks stuck exports
High Severity
The SQL, archives, and finalize tasks no longer mark exports as FAILED in their exception handlers, relying entirely on the cleanup task via Celery's link_error mechanism. If the cleanup task doesn't execute due to infrastructure issues, Celery bugs, or error callback failures, exports will remain permanently stuck in IN_PROGRESS status. Known Celery issues with chord error callbacks make this particularly risky.
Additional Locations (2)
There was a problem hiding this comment.
Given the scope of how often this task will run, it's okay for it to rely on the cleanup task for this
Adjust owner_id to ownerid to adhere to task params
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Note
Aligns owner export tasks to a single
owneridparameter and tightens error/cleanup flows.owner_id→owneridinTaskService.export_owner_data, admin trigger, task kwargs/logging, and all worker task implementations (ExportOwnerTask,ExportOwnerSQLTask,ExportOwnerArchivesTask,ExportOwnerFinalizeTask,ExportOwnerCleanupTask)export_owner_task_nameto routing/plan resolution so queue selection usesownerid_mark_export_failed_by_idatomic (updates only when status isIN_PROGRESS)ownerid-derived pathsowneridconsistentlyWritten by Cursor Bugbot for commit 8d94926. This will update automatically on new commits. Configure here.