Reduce DB contention: skip project UPDATE when registering new files - #1813
Reduce DB contention: skip project UPDATE when registering new files#1813valyo wants to merge 30 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1813 +/- ##
==========================================
+ Coverage 92.91% 92.93% +0.02%
==========================================
Files 30 30
Lines 5052 5069 +17
==========================================
+ Hits 4694 4711 +17
Misses 358 358 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Looking at this now (sorry it's taking time) and so far I don't see a big issue and I think you have found smart solutions. I will have a more thorough look though. One thing I'd like though first is for a more detailed description of the problem. So what is the PR trying to do and why? I know you have mentioned it a bit but could you write it a bit more clearly in case someone aside from us reads this at some point? Should basically just need a little restructuring and rephrasing. |
big thanks! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 101a676256
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new_file.project_id = project.id | ||
| new_version.project_id = project.id |
There was a problem hiding this comment.
By setting project_id directly here, POST /file/new no longer touches the projects row, so concurrent requests for the same project are no longer serialized. In this code path, duplicate prevention is still an application-level precheck (verify_file_not_exists) and files has no DB uniqueness constraint on (project_id, name) (see dds_web/database/models.py), so two simultaneous registrations of the same path can both pass validation and insert duplicate active file rows. That creates data-integrity issues (later PUT/delete paths use .first() and may operate on an arbitrary duplicate).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
To clarify, the race actually existed pre-PR too: the UPDATE projects from the cascade serializes commits but not the SELECT-vs-INSERT ordering, so two workers can both pass verify_file_not_exists and both insert under REPEATABLE READ. This PR widens the race rather than introducing it.
Either way, the right fix is a DB-level UNIQUE (project_id, name) on files, with the schema-level precheck kept as a fast path and a try/except IntegrityError → FileExistsError at commit. Happy to do this in a follow-up PR or fold it in here — let me know which is preferred. Need to confirm "delete" semantics first (hard vs soft) since that affects whether a partial constraint is needed.
Great, thanks! |
i-oden
left a comment
i-oden
left a comment
There was a problem hiding this comment.
Sorry for the long wait again. I've looked again now and I've tested it out a bit. Most of it acted like I was expecting but one thing didn't.
I first tested dds_web/dev with dds_cli/dev (the baseline) and the projects.last_updated was updated when there's a file upload and a file update.
Then I tested this branch with dds_cli/dev and the projects.last_updated was not updated when there's a file upload (as expected) but it's updated when there's a file update i.e. when e.g. overwrite flag is used
<<<
Then I also ofc tested this branch with the dds_cli/improve-project-updating... branch and that seems to work regarding updating the projects.last_updated at the end.
Only using the specific column here because that's where I think we really notice it at all.
So what I'm wondering is if there should be some change in the --overwrite process here too?
…r precise assertion
@i-oden : the |
Great, I'll have another look! |
i-oden
left a comment
i-oden
left a comment
There was a problem hiding this comment.
A new review round
As I've said I think you've come up with a really good solution and it all looks very good, just some questions and notes, and potentially a few more tests to verify the behavior.
Questions:
- Should the tests for the new endpoint be in the
test_files_new.py?
Edge cases:
- Project status changes during upload means the
date_updatedwill not be updated -- this is already a potential issue during upload so nothing that really applies to this specific change. May need a solution at some point but not in this task.
Potential additional tests:
ProjectUploadCompletedoes not update if the project is not In ProgressProjectUploadCompletehandling of db failures- Missing
projectin query toProjectUploadComplete NewFilePOSTno longer updatesproject.date_updated/last_updated_byNewFilePUTstill creates a Version tied to both the file and the project
| project.file_versions.append(new_version) | ||
| # Set FK directly so we do not modify the project row (avoids UPDATE on projects | ||
| # and reduces lock contention during concurrent PUT /file/new). | ||
| new_version.project_id = project.id |
There was a problem hiding this comment.
Not a blocker:
We don't need both
new_version = models.Version(
[...]
project_id=project,
)above and
new_version.project_id = project.idNot a huge deal, but might be confusing in the future.
There are 2 alternatives I think (I tested locally) work the same way without us repeating the project connection:
- remove the
new_version.project_id = project.idrow and changeprojecttoproject.idin the version:# New version new_version = models.Version( size_stored=file_info.get("size_processed"), time_uploaded=new_timestamp, active_file=existing_file.id, project_id=project.id, ) existing_file.versions.append(new_version)
- Remove the
project_id=projectin the version definition and keep thenew_version.project_id = project.id.# New version new_version = models.Version( size_stored=file_info.get("size_processed"), time_uploaded=new_timestamp, active_file=existing_file.id, ) # Set FK directly so we do not modify the project row (avoids UPDATE on projects # and reduces lock contention during concurrent PUT /file/new). new_version.project_id = project.id existing_file.versions.append(new_version)
There was a problem hiding this comment.
Thanks @i-oden , all your comments are addressed now
2 participants
Pull Request Template
Before Marking as Ready for Review
dev(or other targeted branch)SPRINTLOG.mdif neededIf the target branch is
master:Summary
Removes the
UPDATE projectsrow write that previously fired on everyPOST /file/new, eliminating the per-file lock contention thatserialized concurrent file inserts on the same project.
Problem
POST /file/newregisters one file in the database. Internally, theNewFileSchemaattached the new file to the project via collectionappends:
SQLAlchemy treats those appends as mutations of the parent
Projectrow, so each call issued an
UPDATE projects ...alongside theINSERT filesandINSERT versions. With many parallel uploads in asingle
dds data putrun, everyPOST /file/newhad to acquire awrite lock on the same project row, serializing what should have been
independent inserts and slowing large uploads noticeably.
Fix
Set the foreign keys directly instead of going through the parent's
collection:
The project row is no longer dirtied, so no
UPDATE projectsisemitted and concurrent file inserts no longer contend on its row lock.
Because the file is no longer attached to the session via the
project-cascade,
api/files.pyadds it explicitly:The version is still attached via
new_file.versions.append(new_version)through the File→Version cascade.
New endpoint:
POST /proj/upload/completeThe project row's
date_updated(andlast_updated_by) used to berefreshed as a side-effect of the per-file
UPDATE projects. Withthat gone, project metadata would otherwise stop reflecting upload
activity. To preserve that signal — but at one write per batch instead
of one per file — this PR adds:
POST /proj/upload/complete?project=<public_id>The CLI calls it once at the end of
dds data putwhen at least onefile was registered. Auth:
Unit AdminandUnit Personnel(matchingthe rest of the upload surface — Project Owners are not upload-eligible).
Backwards compatibility
Older CLIs continue to work unchanged; they just no longer call
/proj/upload/complete, which meansproject.date_updatedwill nottick during their upload runs until they upgrade. Once the matching
CLI version is published, all timestamps catch up automatically.
Tests
test_proj_upload_complete_updates_timestamp(v1 and v3): happy-pathtest that the new endpoint refreshes
date_updatedandlast_updated_by.test_init.py:/proj/upload/completeblocked under maintenance,and 403 for super-admin-without-role coverage.
test_new_filecontinues to verify the schema changeend-to-end.
What to look at in review
dds_web/api/schemas/file_schemas.py— the schema mutationis the heart of the change.
dds_web/api/files.py— the explicitdb.session.add(new_file)must be in place; without it, removing the collection append would
silently drop the file.
dds_web/api/project.py— the new resource class.Related Issue/Ticket
dds_cli #942
Testing
If applicable: How did you verify the change? Include commands, data, or screenshots.
Reviewer Notes
Anything that helps reviewers (e.g. areas needing close attention).
Once all boxes are checked, mark the PR as Ready for Review and tag at least one team member as the initial reviewer.