check uploads for notify #558

maxweng-sentry · 2025-11-11T18:22:31Z

https://linear.app/getsentry/issue/CCMRG-1821/we-should-not-let-the-notify-task-run-if-there-are-still-pending

Checks to see if there are any uploads still pending before triggering the notify task. Also cleans up any stalled uploads so that future uploads can successfully finish.

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.

sentry · 2025-11-12T00:32:18Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.87%. Comparing base (519a0a9) to head (5bdcbd7).
⚠️ Report is 26 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #558      +/-   ##
==========================================
+ Coverage   93.85%   93.87%   +0.01%     
==========================================
  Files        1284     1284              
  Lines       46428    46434       +6     
  Branches     1524     1524              
==========================================
+ Hits        43574    43588      +14     
+ Misses       2545     2537       -8     
  Partials      309      309

Flag	Coverage Δ
workerintegration	`58.60% <66.66%> (+0.02%)`	⬆️
workerunit	`91.21% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codecov-notifications · 2025-11-12T00:32:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

ajay-sentry · 2025-11-14T21:26:36Z

apps/worker/tasks/manual_trigger.py

+        state = ProcessingState(repoid, commitid)
+        upload_numbers = state.get_upload_numbers()
+
+        if upload_numbers.processing > 0 or upload_numbers.processed > 0:


processed to me implies that it's "done," e.g. a processing upload turns to a processed one. Is that not the case?

yeah, this is true, but there's another state merged.

To add to this, I think we should have a helper function in state.py that does this logic. Perhaps has_unmerged_uploads

thomasrockhu-codecov · 2025-11-16T20:44:40Z

apps/worker/tasks/upload_finisher.py

        except SoftTimeLimitExceeded:
            log.warning("run_impl: soft time limit exceeded")
+            # Clean up orphaned state so future finishers can proceed
+            state.mark_uploads_as_merged(upload_ids)


I think this is wrong, this will mark all uploads as merged correct? But if we hit a SoftTimeLimitExceeded, I'm guessing this is not true

thomasrockhu-codecov · 2025-11-16T20:52:59Z

apps/worker/tasks/manual_trigger.py

+        state = ProcessingState(repoid, commitid)
+        upload_numbers = state.get_upload_numbers()
+
+        if upload_numbers.processing > 0 or upload_numbers.processed > 0:


yeah, this is true, but there's another state merged.

thomasrockhu-codecov · 2025-11-16T21:22:55Z

apps/worker/tasks/tests/unit/test_upload_finisher_task.py

        assert result == []
+
+    @pytest.mark.django_db
+    def test_upload_finisher_updates_repository_timestamp(


Does this test need to exist as a standalone?

thomasrockhu-codecov · 2025-11-16T21:26:17Z

apps/worker/tasks/manual_trigger.py

+        state = ProcessingState(repoid, commitid)
+        upload_numbers = state.get_upload_numbers()
+
+        if upload_numbers.processing > 0 or upload_numbers.processed > 0:


To add to this, I think we should have a helper function in state.py that does this logic. Perhaps has_unmerged_uploads

apps/worker/tasks/manual_trigger.py

thomasrockhu-codecov · 2025-11-16T21:26:57Z

apps/worker/tasks/manual_trigger.py

+            log.info(
+                "Retrying ManualTriggerTask. Redis shows uploads still being processed.",
+                extra={
+                    "repoid": repoid,


nit: alphabetize

thomasrockhu-codecov · 2025-11-16T21:41:16Z

apps/worker/tasks/manual_trigger.py

            if not upload.state or upload.state_id == UploadState.UPLOADED.db_id:
                still_processing += 1
+
+        # Also check Redis processing state to prevent race conditions


I'm not sure I buy this, I think it makes more sense to use either the DB or Redis as the source of truth even with a race condition.

thomasrockhu-codecov · 2025-11-16T21:42:05Z

apps/worker/tasks/manual_trigger.py

+                    "commitid": commitid,
+                    "redis_processing": upload_numbers.processing,
+                    "redis_processed": upload_numbers.processed,
+                    "db_still_processing": still_processing,


not sure the value this will have if it doesn't equal redis_processing + redis_processed

codecov-eu · 2025-11-18T22:25:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

thomasrockhu-codecov · 2025-11-19T21:27:02Z

apps/worker/tasks/upload_finisher.py


+        # Check if there are still pending uploads in the database
+        # Use DB as source of truth for upload completion status
+        if db_session:


Perhaps this should be a helper function as we call it twice

armenzg · 2025-12-03T19:05:48Z

@sentry review

sentry · 2025-12-03T19:07:36Z

apps/worker/tasks/upload_finisher.py

+            # Use DB as source of truth - if any uploads are still in UPLOADED state,
+            # another finisher will process them and we shouldn't send notifications yet
+            remaining_uploads = (
+                db_session.query(Upload)


The SQLAlchemy filter Upload.report.has(commit=commit) is less explicit than directly referencing the CommitReport model's relationship to Commit. For clarity and robustness, consider changing this to Upload.report.has(CommitReport.commit == commit) or Upload.report.has(CommitReport.commit_id == commit.id_), similar to how CommitReport.commit == commit is used in manual_trigger.py.
_{Severity: MEDIUM}

🤖 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/upload_finisher.py#L323-L326 Potential issue: The SQLAlchemy filter `Upload.report.has(commit=commit)` is less explicit than directly referencing the `CommitReport` model's relationship to `Commit`. For clarity and robustness, consider changing this to `Upload.report.has(CommitReport.commit == commit)` or `Upload.report.has(CommitReport.commit_id == commit.id_)`, similar to how `CommitReport.commit == commit` is used in `manual_trigger.py`.

_{Did we get this right? 👍 / 👎 to inform future reviews.}
_{Reference ID: 5283190}

sentry · 2025-12-03T19:07:36Z

apps/worker/tasks/upload_finisher.py

        processing_results: list[ProcessingResult],
+        db_session=None,
    ) -> ShouldCallNotifyResult:
        extra_dict = {
            "repoid": commit.repoid,


Similar to the query in run_impl, the SQLAlchemy filter Upload.report.has(commit=commit) could be made more explicit. Using Upload.report.has(CommitReport.commit == commit) or Upload.report.has(CommitReport.commit_id == commit.id_) would improve readability and maintainability, aligning with more standard SQLAlchemy practices.
_{Severity: MEDIUM}

🤖 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/upload_finisher.py#L669-L672 Potential issue: Similar to the query in `run_impl`, the SQLAlchemy filter `Upload.report.has(commit=commit)` could be made more explicit. Using `Upload.report.has(CommitReport.commit == commit)` or `Upload.report.has(CommitReport.commit_id == commit.id_)` would improve readability and maintainability, aligning with more standard SQLAlchemy practices.

_{Did we get this right? 👍 / 👎 to inform future reviews.}
_{Reference ID: 5283190}

maxweng-sentry force-pushed the max/notify-fix branch from 042ddf6 to 83f4c24 Compare November 13, 2025 22:59

maxweng-sentry added 3 commits November 13, 2025 15:00

check uploads for notify

53887bf

update tests

3081b77

add test

52a129f

maxweng-sentry force-pushed the max/notify-fix branch from 83f4c24 to 52a129f Compare November 13, 2025 23:04

maxweng-sentry added 2 commits November 13, 2025 15:31

add changes back

027e1f0

one more test

2e2a659

ajay-sentry reviewed Nov 14, 2025

View reviewed changes

thomasrockhu-codecov requested changes Nov 16, 2025

View reviewed changes

maxweng-sentry added 2 commits November 18, 2025 13:29

use db for truth

0f838fd

tests

f6c5aed

cleanup test

5bdcbd7

maxweng-sentry requested a review from thomasrockhu-codecov November 19, 2025 18:33

thomasrockhu-codecov approved these changes Nov 19, 2025

View reviewed changes

maxweng-sentry added this pull request to the merge queue Nov 20, 2025

Merged via the queue into main with commit 5ae548a Nov 20, 2025
39 checks passed

maxweng-sentry deleted the max/notify-fix branch November 20, 2025 18:30

sentry bot reviewed Dec 3, 2025

View reviewed changes

check uploads for notify #558

check uploads for notify #558

Uh oh!

Conversation

maxweng-sentry commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Legal Boilerplate

Uh oh!

sentry bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-notifications bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-eu bot commented Nov 18, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

armenzg commented Dec 3, 2025

Uh oh!

sentry bot Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

sentry bot Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maxweng-sentry commented Nov 11, 2025 •

edited

Loading

sentry bot commented Nov 12, 2025 •

edited

Loading

codecov-notifications bot commented Nov 12, 2025 •

edited

Loading