Repository navigation
Do not start a second review while one is running - #79
maxime-pharmania wants to merge 1 commit into
Conversation
The first-push guard counts completed reviews only, so any event arriving before the running review ends starts a duplicate. GitHub sends opened and then synchronize seconds apart when a commit lands while the pull request is created, and both events passed the guard: two reviews ran in parallel on the same pull request and both published a comment. hasRunningPullRequestReview() counts reviews still in started state, and the GitHub, GitLab and Forgejo review handlers now stop on it. A run left in started by a crash is already reset to failed at boot, so the guard cannot block a repository forever.
![proval-0e6d07[bot]](https://avatars.githubusercontent.com/in/4828992?s=60&v=4){kind=small}
| if (accessSkip) return accessSkip; | ||
|
|
||
| const activityService = new ActivityService(); | ||
| if (await activityService.hasRunningPullRequestReview(repository.id, prNumber)) { |
There was a problem hiding this comment.
⚫️ [PROBLEM] Skipped synchronize in on_every_push is never re-reviewed
In on_every_push mode, a synchronize event that lands while a review is already in flight is skipped with a 200 and never revisited. The head-SHA dedup at apps/api/src/webhook/github/github.controller.ts:180 only looks at completed reviews (findLastReviewedHeadSha filters status = "completed" in apps/api/src/api/activity/activity.service.ts:416), and the completion path in apps/api/src/api/activity/activity.runner.ts:10-22 does not re-check whether the pushed head moved. So for the common "push, review starts, push again minutes later" loop, the second push's head receives no review until a third push arrives — before this change, that synchronize at least started a review. The same guard shape exists in gitlab.controller.ts:147 and forgejo.controller.ts:180.
Suggested direction: only apply the running-guard in on_first_push mode, or remember the skipped head and start a follow-up review when the in-flight one completes.
|
|
||
| const activityService = new ActivityService(); | ||
| const prIid = pullRequest.iid; | ||
| if (await activityService.hasRunningPullRequestReview(repository.id, prIid)) { |
There was a problem hiding this comment.
⚫️ [PROBLEM] Guard is check-then-run; started row lands only after network I/O
The guard is a check-then-run with a wide window: it checks for a status = "started" row here, but that row is only inserted by activityService.start inside runWithActivity (apps/api/src/api/activity/activity.runner.ts:9), which runs fire-and-forget after awaited network I/O in the handler (in the GitHub controller: fetchPullRequestVersion at line 175, then fetchPullRequestChangedFileCount, then runWithActivity at line 208). Two near-simultaneous events — a webhook redelivery, or a push landing while the opened handler is still fetching — both pass this check and both launch reviews, which is the exact duplicate-launch behavior this PR aims to prevent.
Consider reserving the in-flight state synchronously at guard time (e.g. an in-process Set keyed by ${repositoryId}:${targetIid} inserted before the version fetches and cleared in runWithActivity's finally), or inserting the started activity row before the network I/O and reusing it as the run's row. This same window exists in the GitHub and Forgejo controllers.
|
Thanks for adding the in-flight review guard — this closes a real duplicate-review window that the completed-only check left open, and the new test suite exercises the actual SQLite query semantics well. a. Summary This PR adds b. Good points
c. Main Issues
d. Additional Notes
|
| /** | ||
| * True while a pull request review of this repository and pull request is | ||
| * still running. The completed check alone lets a second event started | ||
| * before the first review ends launch a duplicate review: GitHub sends | ||
| * "opened" and a following "synchronize" seconds apart, and both would pass. | ||
| * A run left in "started" by a crash is reset to "failed" at boot. | ||
| */ | ||
| public async hasRunningPullRequestReview(repositoryId: number, targetIid: number): Promise<boolean> { | ||
| const [{ total }] = await db | ||
| .select({ total: count() }) | ||
| .from(activityTable) | ||
| .where( | ||
| and( | ||
| eq(activityTable.repositoryId, repositoryId), | ||
| eq(activityTable.targetIid, targetIid), | ||
| eq(activityTable.type, "pr_review"), | ||
| eq(activityTable.status, "started"), | ||
| ), | ||
| ); | ||
| return total > 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
This PR looks great! But I think checking in-flight PR review from database is not the best approach. Maybe storing the in-flight PRs in the memory and checking them from memory is better.
|
This is really a bug. Thanks for reporting and making PRs for this. Proval actually got some guards about this, but it only checks if there is a Your The tradeoff is that option with I added a small change on your branch. treat So we still avoid duplicate full reviews, but we do not drop every mid run push. We may run an extra smaller incremental review instead. A follow up comment might appear before the first one finishes. That is the tradeoff we accept. |
|
@maxime-pharmania I can't push my commit to your fork (https://github.com/pharmania/proval.git) Could you add me as a collaborator on that fork (or enable maintainer edits on your PRs) so I can push? I pushed my updates to my repo so you can check my updates |
3 participants
The review guard reads completed reviews only, so two GitHub events that are seconds apart both pass it and start a full review each. Seen in production on a pull request that was opened and then received a commit 11 seconds later: activity 1 (created 4 seconds after the pull request) and activity 2 (created 18 seconds later) ran in parallel for ~56 minutes, consumed ~10.7M and ~11.8M input tokens, and both published a review plus inline comments on the same pull request. The same pull request also shows two activities pointing at the same head, 2 seconds apart.
hasRunningPullRequestReview()counts reviews still instartedstate for that repository and pull request.on_first_pushandon_every_push.startedby a crash is already reset tofailedat boot (apps/api/src/index.ts), so the guard cannot block a repository forever.Tests (
apps/api/src/api/activity/pr-review-guard.test.ts, real in-memory database, isolated child process like the other integration tests): a review in flight is reported; ended reviews of any outcome are ignored; another activity type, another pull request and another repository are ignored; andhasCompletedPullRequestReview()stays false while the only review of that pull request is running, which is the exact gap this closes.This is a best-effort guard, not a lock: the two events are read before the activity row is created, so a sub-millisecond interleaving could still pass. Compared to a review that runs for minutes, that window is negligible.