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

Don't use .save() in validate_version_metadata #1800

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

jjnesbitt
Copy link
Member

@jjnesbitt jjnesbitt commented Dec 30, 2023

We're seeing a very prevalent race condition, resulting in version asset summary aggregations being skipped (this can be observed via copious amounts of Skipped updating assetsSummary for version *** in the papertrail logs).

Here's what I believe is happening:

  1. A version is updated in some way, and status set to PENDING
  2. The scheduled task validate_draft_version_metadata is run, dispatching the validate_version_metadata and aggregate_assets_summary tasks at the same time
  3. validate_version_metadata finishes before aggregate_assets_summary, calling Version.save(), which calls Version._populate_metadata, potentially modifying the metadata
  4. aggregate_assets_summary finishes generating the summary and reaches this line, which will only update the version if the metadata field hasn't changed from when the task was dispatched. Since the above step modifies the metadata, this filter fails and the update is skipped
  5. The version assets summary remains outdated/incorrect

This PR changes the behavior of validate_version_metadata to use .update when updating the status and validation_errors of a Version, subverting the underlying call to Version._populate_metadata, and thus any metadata changes.

@danlamanna I'd like to hear your input on this, particularly to make sure I'm not violating any intended behavior. I believe I've largely kept in line with what was already there, but I'm unfamiliar with the reason/need for using the metadata=version.metadata filter when updating a version's asset summary.

@jjnesbitt jjnesbitt removed the request for review from mvandenburgh December 30, 2023 19:52
@jjnesbitt
Copy link
Member Author

jjnesbitt commented Dec 30, 2023

I just realized I may need to also include an update to the version modified field. This has been fixed.

@jjnesbitt jjnesbitt added patch Increment the patch version when merged release Create a release when this pr is merged labels Feb 14, 2024
@jjnesbitt jjnesbitt merged commit 4637cb3 into master Feb 14, 2024
10 checks passed
@jjnesbitt jjnesbitt deleted the fix-incorrect-assets-summary branch February 14, 2024 15:05
@dandibot
Copy link
Member

🚀 PR was released in v0.3.76 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Feb 14, 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.

4 participants