Skip to content
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

Retry aggregate_assets_summary_task on version metadata race condition #1803

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

jjnesbitt
Copy link
Member

Fixes #1734

Related to #1800. Both this PR and #1800 address #1734, although this change should cover all sources of a concurrent metadata change, instead of just the validate_version_metadata task.

dandiapi/api/tests/test_version.py Outdated Show resolved Hide resolved
@jjnesbitt jjnesbitt added patch Increment the patch version when merged release Create a release when this pr is merged labels Jan 16, 2024
@@ -95,6 +96,7 @@ def version_aggregate_assets_summary(version: Version) -> None:
)
if updated_count == 0:
logger.info('Skipped updating assetsSummary for version %s', version.id)
raise VersionMetadataConcurrentlyModified
Copy link
Member

Choose a reason for hiding this comment

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

Will this trigger a Sentry error? I imagine we wouldn't want that to happen in this case.

Copy link
Member Author

@jjnesbitt jjnesbitt Jan 16, 2024

Choose a reason for hiding this comment

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

I don't think it will. I believe what happens is the following:

  1. This exception is raised
  2. Since the task is decorated with autoretry_for, it catches that exception and raises a celery Retry exception (documented here)
  3. Sentry (specific the celery integration) sees the Retry exception is raised, but ignores it, as it's recognized as part of celery's control flow. Here is the code that controls that.

So we should be good, but I'll do a little more testing to make sure things behave as we expect before merging.

@jjnesbitt jjnesbitt merged commit 0367f54 into master Jan 16, 2024
10 checks passed
@jjnesbitt jjnesbitt deleted the retry-assets-summary branch January 16, 2024 18:58
@dandibot
Copy link
Member

🚀 PR was released in v0.3.72 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asset metadata not appearing on DLP
4 participants