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

fix: OPTIC-1438: Locking summary for update before reset #7127

Merged
merged 6 commits into from
Feb 25, 2025

Conversation

mcanu
Copy link
Contributor

@mcanu mcanu commented Feb 21, 2025

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

I've identified a potential race condition on data reimport.

  1. Background worker is processing the imported data and creating tasks in a transaction
  2. At the same time the API container receives a request to validate the project labeling configuration
  3. The validate_config method of the project checks the number of tasks==0, so proceeds to reset the summary
  4. The background worker updates the summary all_data_columns with the correct columns
  5. The API process resets the summary

This PR adds a select_for_update on the summary before the creation of tasks and before checking the number of tasks on a potential reset. This should lock one of them out, not allowing the scenario described above.

@github-actions github-actions bot added the fix label Feb 21, 2025
Copy link

sentry-io bot commented Feb 21, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: label_studio/data_import/functions.py

Function Unhandled Issue
async_import_background KeyError: 'predictions' data_import.functions.asy...
Event Count: 42
async_reimport_background [**ValidationError: ["Failed to parse input file 028a36e7-export_126647_project-126647-at-2025-02-13-20-14-6a27ccba.csv: ErrorDetail(string='Your label config has more than one data key and direct file upload supports only one data key. To import data with multiple data k...** ...
Event Count: 2
async_import_background ValidationError: ["HTTPConnectionPool(host='127.0.0.1', port=5050): Max retries exceeded with url: / (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f6376ff2f30>: Failed to establish a new connection: [Errno 111] Connection refused'))"] ...
Event Count: 1
async_import_background ValidationError: ["Failed to parse input file ab4fd004-labelstudio_test.json: Unexpected character in found when decoding object value"] ...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

Copy link

netlify bot commented Feb 21, 2025

Deploy Preview for heartex-docs canceled.

Name Link
🔨 Latest commit db9411b
🔍 Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/67bde911190eda0008c588c3

Copy link

netlify bot commented Feb 21, 2025

Deploy Preview for label-studio-docs-new-theme canceled.

Name Link
🔨 Latest commit db9411b
🔍 Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/67bde911df21f900085caac2

@mcanu mcanu requested review from a team and bmartel February 21, 2025 21:23
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 51.06383% with 23 lines in your changes missing coverage. Please review.

Project coverage is 77.10%. Comparing base (7666677) to head (db9411b).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
label_studio/data_import/functions.py 4.54% 21 Missing ⚠️
label_studio/projects/models.py 92.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7127      +/-   ##
===========================================
- Coverage    77.11%   77.10%   -0.01%     
===========================================
  Files          183      183              
  Lines        14414    14421       +7     
===========================================
+ Hits         11116    11120       +4     
- Misses        3298     3301       +3     
Flag Coverage Δ
pytests 77.10% <51.06%> (-0.01%) ⬇️

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.

@mcanu
Copy link
Contributor Author

mcanu commented Feb 21, 2025

/fm sync

Workflow run

@mcanu
Copy link
Contributor Author

mcanu commented Feb 24, 2025

/git merge develop

Workflow run
Successfully merged: create mode 100644 web/libs/core/src/types/user.ts

Copy link
Contributor

@bmartel bmartel left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'd be curious as to how this influences performance with regards to importing tasks. Overall it cannot be escaped that this must adhere to a certain logical guarantee, otherwise there are potential race conditions which could occur.

@mcanu
Copy link
Contributor Author

mcanu commented Feb 25, 2025

@bmartel completely agree, I'll monitor performance around this to see the impact this has.

@mcanu
Copy link
Contributor Author

mcanu commented Feb 25, 2025

/git merge develop

Workflow run
Successfully merged: 16 files changed, 121 insertions(+), 32 deletions(-)

@mcanu
Copy link
Contributor Author

mcanu commented Feb 25, 2025

/git merge develop

Workflow run
Successfully merged: 9 files changed, 27 insertions(+), 16 deletions(-)

@mcanu mcanu merged commit 7b9b35c into develop Feb 25, 2025
41 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants