Fix-copr-builds-race-condition #2982
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a race condition when creating Copr build groups, particularly in concurrent scenarios with monorepos. The use of an atomic database update is a solid approach to prevent this issue. The changes are well-supported by a comprehensive, albeit complex, new test case that simulates the race condition. The updates to other files are mostly mechanical adaptations to the new method signature. I have one suggestion regarding code duplication that could improve maintainability.
nforro
left a comment
nforro
left a comment
There was a problem hiding this comment.
The wording revolves around threads, but in deployment it could be different workers causing the issue. I would probably use the term worker which can stand for both a worker container and a worker thread.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 53s |
|
❌ pre-commit FAILURE in 1m 51s |
If you run the test, it will fail with a race condition. If you comment out the barrier.wait(), and add a sleep of 2 seconds before each thread starts, the test will "pass", or better, it will fail, differently, because the threads didn't run at the same time.
It fixes the race condition triggered by the unit test created in the previous commit. Co-authored-by: Nikola Forró <[email protected]>
majamassarini
force-pushed
the
fix-copr-builds-race-condition
branch
from
613737a to
7e99633
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a race condition in Copr build group creation. The new implementation in CoprBuildGroupModel.create correctly uses atomic database operations to prevent concurrent workers from causing issues, which is a solid approach. The addition of a comprehensive, multi-threaded test case to reproduce and verify the fix is excellent and greatly increases confidence in the change. I have a couple of suggestions to improve the implementation of the new test case for better maintainability and robustness, but the core logic of the fix is sound.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 52s |
2 participants
It fixes the race condition triggered by the unit test created in the previous commit.
RELEASE NOTES BEGIN
We have fixed a race condition that prevented copr builds to being reported as complete.
RELEASE NOTES END