fix: use sync.Once around create_comment API call#11
Conversation
Summary of ChangesHello @zakisk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where multiple identical comments could be created on GitHub pull requests due to concurrent processing. By integrating Go's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 aims to replace a fragile, sleep-based workaround for a race condition with a more robust sync.Once mechanism to prevent duplicate comment creation on GitHub Pull Requests. However, the current implementation introduces significant security and functional flaws. It causes a permanent memory leak due to the global sync.Map lacking an eviction strategy, which can lead to resource exhaustion. Furthermore, it incorrectly suppresses all subsequent comments on a PR after the first one is attempted for the lifetime of the process, potentially hiding critical status or security notifications, and leads to false positive success reports in concurrent scenarios. Additionally, the condition for using the synchronization logic is too restrictive, leaving some pull request-related events unprotected from the race condition.
| var once *sync.Once | ||
| var err error | ||
| if event.TriggerTarget == triggertype.PullRequest { | ||
| key := fmt.Sprintf("%s/%s/%d", event.Organization, event.Repository, event.PullRequestNumber) | ||
| value, _ := syncMap.LoadOrStore(key, &sync.Once{}) | ||
| var ok bool | ||
| once, ok = value.(*sync.Once) | ||
| if !ok { | ||
| return fmt.Errorf("unexpected type in sync map for key %s", key) | ||
| } | ||
| } else { | ||
| once = &sync.Once{} | ||
| } | ||
|
|
||
| _, _, err := wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) { | ||
| return v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{ | ||
| Body: github.Ptr(commit), | ||
| once.Do(func() { | ||
| _, _, err = wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) { | ||
| return v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{ | ||
| Body: github.Ptr(commit), | ||
| }) | ||
| }) | ||
| }) | ||
| return err |
There was a problem hiding this comment.
The synchronization logic using sync.Once per Pull Request is fundamentally flawed. It leads to permanent suppression of comments because sync.Once instances are never cleared from the global map, causing CreateComment to execute only once per PR for the controller's lifetime. This silently skips critical status updates and security notifications. Additionally, it can result in false positive success reports: if multiple goroutines call CreateComment concurrently and the API call fails for the executing goroutine, others will return nil (success) because their local err variable remains uninitialized. Furthermore, the check if event.TriggerTarget == triggertype.PullRequest is too restrictive. Events like /retest or /ok-to-test comments, which also operate on pull requests, would use a transient sync.Once, failing to prevent the intended race condition. The logic should be simplified to always use the sync.Map for sync.Once instances, ensuring all concurrent comment creation attempts for the same PR are synchronized regardless of the specific trigger type.
var err error
key := fmt.Sprintf("%s/%s/%d", event.Organization, event.Repository, event.PullRequestNumber)
value, _ := syncMap.LoadOrStore(key, &sync.Once{})
once, ok := value.(*sync.Once)
if !ok {
return fmt.Errorf("unexpected type in sync map for key %s", key)
}|
|
||
| var _ provider.Interface = (*Provider)(nil) | ||
|
|
||
| var syncMap = sync.Map{} |
There was a problem hiding this comment.
The syncMap global variable, used to store sync.Once instances for each unique Pull Request, creates a permanent memory leak as entries are never removed. This leads to indefinite memory growth, resource exhaustion, and potential Denial of Service (OOM) in long-running processes. To mitigate this, consider implementing a cache with a time-to-live (TTL) or a size-based eviction policy (like LRU) for sync.Once objects. Alternatively, a cleanup mechanism could be added, such as removing entries when pull requests are closed or merged.
zakisk
force-pushed
the
fix-duplicate-comments-in-e2e
branch
from
fd8ae01 to
5720faf
Compare
Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
zakisk
force-pushed
the
fix-duplicate-comments-in-e2e
branch
from
5720faf to
8056245
Compare
this commit uses sync.Once to make sure that create_comment is called only once. it stores the sync.Once instance for every unique PR in sync.Map and retrieves and call the API in it. Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
zakisk
force-pushed
the
fix-duplicate-comments-in-e2e
branch
from
8056245 to
ea008b5
Compare
zakisk
force-pushed
the
main
branch
2 times, most recently
from
decf4c1 to
0faad24
Compare
1 participant
this commit uses sync.Once to make sure that create_comment is called only once. it stores the sync.Once instance for every unique PR in sync.Map and retrieves and call the API in it.
📝 Description of the Change
👨🏻 Linked Jira
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.