Skip to content

fix: use sync.Once around create_comment API call #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zakisk wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: use sync.Once around create_comment API call #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19b1b2d
add my own second controller config
zakisk Feb 19, 2026
ea008b5
fix: use sync.Once around create_comment API call
zakisk Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ jobs:
TEST_GITHUB_REPO_OWNER_WEBHOOK: openshift-pipelines/pipelines-as-code-e2e-tests-webhook
TEST_GITHUB_SECOND_API_URL: ghe.pipelinesascode.com
TEST_GITHUB_SECOND_EL_URL: http://ghe.paac-127-0-0-1.nip.io
TEST_GITHUB_SECOND_REPO_INSTALLATION_ID: 1
TEST_GITHUB_SECOND_REPO_OWNER_GITHUBAPP: pipelines-as-code/e2e
TEST_GITHUB_SECOND_REPO_INSTALLATION_ID: 8
TEST_GITHUB_SECOND_REPO_OWNER_GITHUBAPP: pac/zaki-test-pac
TEST_GITHUB_SECOND_TOKEN: ${{ secrets.TEST_GITHUB_SECOND_TOKEN }}
TEST_GITHUB_TOKEN: ${{ secrets.GH_APPS_TOKEN }}
TEST_GITLAB_API_URL: https://gitlab.com
Expand Down
152 changes: 30 additions & 122 deletions pkg/provider/github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ const (

var _ provider.Interface = (*Provider)(nil)

var syncMap = sync.Map{}

@gemini-code-assist gemini-code-assist Bot Feb 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

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.


type Provider struct {
ghClient *github.Client
Logger *zap.SugaredLogger
Expand Down Expand Up @@ -792,13 +794,6 @@ func compactCommentIDs(comments []*github.IssueComment) []string {
return out
}

func responseStatusCode(resp *github.Response) int {
if resp == nil {
return 0
}
return resp.StatusCode
}

func eventID(event *info.Event) string {
if event == nil || event.Request == nil {
return "unknown"
Expand Down Expand Up @@ -892,69 +887,6 @@ func (v *Provider) listCommentsByMarker(
return matchedComments, nil
}

func (v *Provider) ensureSingleMarkerComment(
ctx context.Context,
event *info.Event,
comments []*github.IssueComment,
commit string,
trace commentTraceLogContext,
) error {
if len(comments) == 0 {
return nil
}

if len(comments) > 1 {
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(comments))
}

primaryComment := comments[0]
for _, comment := range comments {
if comment.GetBody() == commit {
primaryComment = comment
break
}
}

v.debugCommentPhase(event, trace, "dedup_select_primary",
"matched_count", len(comments),
"primary_comment_id", primaryComment.GetID(),
)

if primaryComment.GetBody() != commit {
if _, _, err := wrapAPI(v, "edit_comment", func() (*github.IssueComment, *github.Response, error) {
return v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, primaryComment.GetID(), &github.IssueComment{
Body: github.Ptr(commit),
})
}); err != nil {
return err
}
}

// Best-effort cleanup to collapse duplicates into a single canonical marker comment.
for _, comment := range comments {
if comment.GetID() == primaryComment.GetID() {
continue
}

v.debugCommentPhase(event, trace, "dedup_delete_attempt", "delete_comment_id", comment.GetID())
_, resp, err := wrapAPI(v, "delete_comment", func() (struct{}, *github.Response, error) {
resp, err := v.Client().Issues.DeleteComment(ctx, event.Organization, event.Repository, comment.GetID())
return struct{}{}, resp, err
})
v.debugCommentPhase(event, trace, "dedup_delete_done",
"delete_comment_id", comment.GetID(),
"status_code", responseStatusCode(resp),
"delete_error", err != nil,
)
if err != nil && v.Logger != nil {
v.Logger.Warnf("failed to delete duplicate comment %d on %s/%s#%d: %v",
comment.GetID(), event.Organization, event.Repository, event.PullRequestNumber, err)
}
}
v.debugCommentPhase(event, trace, "dedup_complete", "final_expected_count", 1)
return nil
}

// CreateComment creates a comment on a Pull Request.
func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit, updateMarker string) error {
if v.ghClient == nil {
Expand All @@ -975,69 +907,45 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,

if len(existingComments) > 1 {
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(existingComments))
if _, _, err := wrapAPI(v, "edit_comment", func() (*github.IssueComment, *github.Response, error) {
return v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, existingComments[0].GetID(), &github.IssueComment{
Body: github.Ptr(commit),
})
}); err != nil {
return err
}
return nil
}

if len(existingComments) > 0 {
return v.ensureSingleMarkerComment(ctx, event, existingComments, commit, trace)
}

//nolint:gosec // No need for crypto/rand here, just reducing timing window
jitter := time.Duration(rand.Intn(500)) * time.Millisecond
v.debugCommentPhase(event, trace, "jitter_wait", "jitter_ms", jitter.Milliseconds())
timer := time.NewTimer(jitter)
defer timer.Stop()

select {
case <-ctx.Done():
return ctx.Err()
case <-timer.C:
}

// Re-check after jitter in case another processor already created the marker comment.
existingComments, err = v.listCommentsByMarker(ctx, event, updateMarker, "post_jitter_list", trace)
if err != nil {
return err
}
if len(existingComments) > 1 {
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(existingComments))
}
if len(existingComments) > 0 {
return v.ensureSingleMarkerComment(ctx, event, existingComments, commit, trace)
}

v.debugCommentPhase(event, trace, "pre_create_race_window", "matched_count", len(existingComments))
}

v.debugCommentPhase(event, trace, "create_comment_start")
createdComment, createResp, err := wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) {
_, _, 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),
})
})
if err != nil {
v.debugCommentPhase(event, trace, "create_comment_done",
"status_code", responseStatusCode(createResp),
"create_error", err.Error(),
)
return err
}
v.debugCommentPhase(event, trace, "create_comment_done",
"status_code", responseStatusCode(createResp),
"created_comment_id", createdComment.GetID(),
)

if updateMarker == "" {
return nil
var once *sync.Once
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{}
}

// Best-effort post-create reconciliation to collapse duplicates created by
// concurrent processors handling the same event.
matchedComments, listErr := v.listCommentsByMarker(ctx, event, updateMarker, "post_create_list", trace)
if listErr != nil {
return nil
}
if len(matchedComments) > 1 {
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(matchedComments))
}
return v.ensureSingleMarkerComment(ctx, event, matchedComments, commit, trace)
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
}
Loading