git-workflow/autogits
SHA256
22
2
Fork 15
Code Issues 44 Pull Requests 17 Actions Projects 2 Activity

WIP: fix: reviews may be ahead of timeline #138

Draft
andriinikitin wants to merge 2 commits from fix-multiple-reviews-from-the-same-user into main
pull from: fix-multiple-reviews-from-the-same-user
merge into: git-workflow:main
Conversation 1 Commits 2 Files Changed 4 +15 -17
andriinikitin commented 2026-02-25 12:35:32 +01:00
Member
Copy Link

The problem may be caused by unlucky timing when a review is submitted right after the bot has read the timeline of some PR.
So the bot sees that all reviews are complete, but the timeline is missing some of entries.
-> The bot thinks that the reviews need to be requested.

Also correctly handle multiple reviews from the same user.

The problem may be caused by unlucky timing when a review is submitted right after the bot has read the timeline of some PR. So the bot sees that all reviews are complete, but the timeline is missing some of entries. -> The bot thinks that the reviews need to be requested. Also correctly handle multiple reviews from the same user.
andriinikitin added 1 commit 2026-02-25 12:35:32 +01:00
fix: correctly handle multiple reviews from the same user
All checks were successful
go-generate-check / go-generate-check (pull_request) Successful in 18s
Details
Integration tests / t (pull_request) Successful in 6m24s
Details
93aab6454d
andriinikitin changed title from fix: correctly handle multiple reviews from the same user to WIP: fix: correctly handle multiple reviews from the same user 2026-02-25 12:44:31 +01:00
andriinikitin added 1 commit 2026-02-25 14:51:44 +01:00
fix reviews may be ahead of timeline
All checks were successful
go-generate-check / go-generate-check (pull_request) Successful in 25s
Details
Integration tests / t (pull_request) Successful in 7m23s
Details
ce15a61eeb 
The bot may be confused if he has all the reviews, but some of them are missing
on the timeline. This may happen if a new review arrives inbetween of gitea api call.
Changing orders of API calls ensures that state of reviews is not ahead of timeline.
andriinikitin changed title from WIP: fix: correctly handle multiple reviews from the same user to fix: correctly handle multiple reviews from the same user 2026-02-25 14:59:40 +01:00
andriinikitin changed title from fix: correctly handle multiple reviews from the same user to fix: reviews may be ahead of timeline 2026-02-25 15:00:49 +01:00
andriinikitin force-pushed fix-multiple-reviews-from-the-same-user from ce15a61eeb to 833b679c96 2026-02-25 15:37:42 +01:00 Compare
adamm reviewed 2026-02-25 17:24:33 +01:00
common/reviews.go
@@ -25,4 +30,0 @@
rawReviews, err := rf.GetPullRequestReviews(org, repo, no)
if err != nil {
return nil, err
}
adamm commented 2026-02-25 17:24:33 +01:00
Owner
Copy Link

I think this is correct. The timeline could be behind rawReviews, but those rawReviews that are not on the timeline should be ignored.

If you fetch timeline after reviews, you could end up with reviews on the timeline that do not exist in the fetched reviews.

I think this is correct. The timeline could be behind rawReviews, but those rawReviews that are *not* on the timeline should be ignored. If you fetch timeline after reviews, you could end up with reviews on the timeline that do not exist in the fetched reviews.
andriinikitin commented 2026-02-25 17:29:07 +01:00
Author
Member
Copy Link

It is two generals problem. With current state the bot will re-request new review(s) if all reviews have been properly submitted and timeline is missing one or some. The reorder fixes that problem in the tests

It is two generals problem. With current state the bot will re-request new review(s) if all reviews have been properly submitted and timeline is missing one or some. The reorder fixes that problem in the tests
adamm commented 2026-02-25 17:32:54 +01:00
Owner
Copy Link

Basically, you should end up in same situation. A race condition, ie. you add a review trigger that is not seen in timeline, means that the timeline is modified asynchronously after requests are added and you still have a race condition.

Or am I missing something in this analysis?

If the bot is requesting data here, it should still see that it requested the review already. So, that would be the bug, if it doesn't see review request.

Basically, you *should* end up in same situation. A race condition, ie. you add a review trigger that is *not* seen in timeline, means that the timeline is modified asynchronously after requests are added and you still have a race condition. Or am I missing something in this analysis? If the bot is requesting data here, it should still see that it requested the review already. So, that would be the bug, if it doesn't see review request.
andriinikitin commented 2026-02-25 17:40:48 +01:00
Author
Member
Copy Link

Yes it is the bug. It it looks that the bot checks the reviews for question 'Are there any pending reviews?' and sees answer "No". Then it checks the timeline and sees that some reviews are missing. It makes conclusion that it needs to request more.

Yes it is the bug. It it looks that the bot checks the reviews for question 'Are there any pending reviews?' and sees answer "No". Then it checks the timeline and sees that some reviews are missing. It makes conclusion that it needs to request more.
adamm commented 2026-02-25 18:02:44 +01:00
Owner
Copy Link

ok, I think I understand where the problem is. Let me make a unit test and fix and you can merge that into this pr?

ok, I *think* I understand where the problem is. Let me make a unit test and fix and you can merge that into this pr?
adamm commented 2026-02-26 20:57:17 +01:00
Owner
Copy Link

I've pushed my changes along with the test changes from this PR to #142 . Can you please take a look and check that this "race condition" is fixed?

In reality, the problem was that issue timeline was not consistently parsed. The initial version of the Reviews code handled only reviews. Then timeline events were tacked on, but not handled correctly. So you could have scenarios where a requested review could not be seen or in another combination, it could be seen the other way. For example, a pending review that is then revoked but was still seen as pending.

I've pushed my changes along with the test changes from this PR to #142 . Can you please take a look and check that this "race condition" is fixed? In reality, the problem was that issue timeline was not consistently parsed. The initial version of the Reviews code handled only reviews. Then timeline events were tacked on, but not handled correctly. So you could have scenarios where a requested review could not be seen or in another combination, it could be seen the other way. For example, a pending review that is then revoked but was still seen as pending.
andriinikitin commented 2026-02-27 02:46:19 +01:00
Author
Member
Copy Link

Can you please take a look and check that this "race condition" is fixed?

I didn't try to understand the changes, but I tested it quite thoroughly in #149 and I confirm that the problem is fixed as it was appearing quite often.

> Can you please take a look and check that this "race condition" is fixed? I didn't try to understand the changes, but I tested it quite thoroughly in #149 and I confirm that the problem is fixed as it was appearing quite often.
andriinikitin changed title from fix: reviews may be ahead of timeline to WIP: fix: reviews may be ahead of timeline 2026-02-27 02:07:07 +01:00
All checks were successful
go-generate-check / go-generate-check (pull_request) Successful in 27s
Details
Integration tests / t (pull_request) Successful in 7m20s
Details
This pull request has changes conflicting with the target branch.
  • integration/test-plan.md
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix-multiple-reviews-from-the-same-user:fix-multiple-reviews-from-the-same-user
git checkout fix-multiple-reviews-from-the-same-user
Sign in to join this conversation.
Reviewers
No Reviewers
adamm
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
M0ses adamm adamm_super adrianSuSE andriinikitin atartamo bigironman dancermak darix dirkmueller dmach eroca jdsn jzerebecki mstrigl oertel smithfarm
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: git-workflow/autogits#138
Delete Branch "fix-multiple-reviews-from-the-same-user"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?