Skip to content

Commit ea008b5

Browse files
committed
fix: use sync.Once around create_comment API call
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>
1 parent 19b1b2d commit ea008b5

1 file changed

Lines changed: 30 additions & 122 deletions

File tree

pkg/provider/github/github.go

Lines changed: 30 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ const (
4444

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

47+
var syncMap = sync.Map{}
48+
4749
type Provider struct {
4850
ghClient *github.Client
4951
Logger *zap.SugaredLogger
@@ -792,13 +794,6 @@ func compactCommentIDs(comments []*github.IssueComment) []string {
792794
return out
793795
}
794796

795-
func responseStatusCode(resp *github.Response) int {
796-
if resp == nil {
797-
return 0
798-
}
799-
return resp.StatusCode
800-
}
801-
802797
func eventID(event *info.Event) string {
803798
if event == nil || event.Request == nil {
804799
return "unknown"
@@ -892,69 +887,6 @@ func (v *Provider) listCommentsByMarker(
892887
return matchedComments, nil
893888
}
894889

895-
func (v *Provider) ensureSingleMarkerComment(
896-
ctx context.Context,
897-
event *info.Event,
898-
comments []*github.IssueComment,
899-
commit string,
900-
trace commentTraceLogContext,
901-
) error {
902-
if len(comments) == 0 {
903-
return nil
904-
}
905-
906-
if len(comments) > 1 {
907-
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(comments))
908-
}
909-
910-
primaryComment := comments[0]
911-
for _, comment := range comments {
912-
if comment.GetBody() == commit {
913-
primaryComment = comment
914-
break
915-
}
916-
}
917-
918-
v.debugCommentPhase(event, trace, "dedup_select_primary",
919-
"matched_count", len(comments),
920-
"primary_comment_id", primaryComment.GetID(),
921-
)
922-
923-
if primaryComment.GetBody() != commit {
924-
if _, _, err := wrapAPI(v, "edit_comment", func() (*github.IssueComment, *github.Response, error) {
925-
return v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, primaryComment.GetID(), &github.IssueComment{
926-
Body: github.Ptr(commit),
927-
})
928-
}); err != nil {
929-
return err
930-
}
931-
}
932-
933-
// Best-effort cleanup to collapse duplicates into a single canonical marker comment.
934-
for _, comment := range comments {
935-
if comment.GetID() == primaryComment.GetID() {
936-
continue
937-
}
938-
939-
v.debugCommentPhase(event, trace, "dedup_delete_attempt", "delete_comment_id", comment.GetID())
940-
_, resp, err := wrapAPI(v, "delete_comment", func() (struct{}, *github.Response, error) {
941-
resp, err := v.Client().Issues.DeleteComment(ctx, event.Organization, event.Repository, comment.GetID())
942-
return struct{}{}, resp, err
943-
})
944-
v.debugCommentPhase(event, trace, "dedup_delete_done",
945-
"delete_comment_id", comment.GetID(),
946-
"status_code", responseStatusCode(resp),
947-
"delete_error", err != nil,
948-
)
949-
if err != nil && v.Logger != nil {
950-
v.Logger.Warnf("failed to delete duplicate comment %d on %s/%s#%d: %v",
951-
comment.GetID(), event.Organization, event.Repository, event.PullRequestNumber, err)
952-
}
953-
}
954-
v.debugCommentPhase(event, trace, "dedup_complete", "final_expected_count", 1)
955-
return nil
956-
}
957-
958890
// CreateComment creates a comment on a Pull Request.
959891
func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit, updateMarker string) error {
960892
if v.ghClient == nil {
@@ -975,69 +907,45 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
975907

976908
if len(existingComments) > 1 {
977909
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(existingComments))
910+
if _, _, err := wrapAPI(v, "edit_comment", func() (*github.IssueComment, *github.Response, error) {
911+
return v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, existingComments[0].GetID(), &github.IssueComment{
912+
Body: github.Ptr(commit),
913+
})
914+
}); err != nil {
915+
return err
916+
}
917+
return nil
978918
}
979-
980-
if len(existingComments) > 0 {
981-
return v.ensureSingleMarkerComment(ctx, event, existingComments, commit, trace)
982-
}
983-
984-
//nolint:gosec // No need for crypto/rand here, just reducing timing window
985-
jitter := time.Duration(rand.Intn(500)) * time.Millisecond
986-
v.debugCommentPhase(event, trace, "jitter_wait", "jitter_ms", jitter.Milliseconds())
987-
timer := time.NewTimer(jitter)
988-
defer timer.Stop()
989-
990-
select {
991-
case <-ctx.Done():
992-
return ctx.Err()
993-
case <-timer.C:
994-
}
995-
996-
// Re-check after jitter in case another processor already created the marker comment.
997-
existingComments, err = v.listCommentsByMarker(ctx, event, updateMarker, "post_jitter_list", trace)
998-
if err != nil {
999-
return err
1000-
}
1001-
if len(existingComments) > 1 {
1002-
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(existingComments))
1003-
}
1004-
if len(existingComments) > 0 {
1005-
return v.ensureSingleMarkerComment(ctx, event, existingComments, commit, trace)
1006-
}
1007-
1008-
v.debugCommentPhase(event, trace, "pre_create_race_window", "matched_count", len(existingComments))
1009919
}
1010920

1011-
v.debugCommentPhase(event, trace, "create_comment_start")
1012-
createdComment, createResp, err := wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) {
921+
_, _, err := wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) {
1013922
return v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{
1014923
Body: github.Ptr(commit),
1015924
})
1016925
})
1017926
if err != nil {
1018-
v.debugCommentPhase(event, trace, "create_comment_done",
1019-
"status_code", responseStatusCode(createResp),
1020-
"create_error", err.Error(),
1021-
)
1022927
return err
1023928
}
1024-
v.debugCommentPhase(event, trace, "create_comment_done",
1025-
"status_code", responseStatusCode(createResp),
1026-
"created_comment_id", createdComment.GetID(),
1027-
)
1028929

1029-
if updateMarker == "" {
1030-
return nil
930+
var once *sync.Once
931+
if event.TriggerTarget == triggertype.PullRequest {
932+
key := fmt.Sprintf("%s/%s/%d", event.Organization, event.Repository, event.PullRequestNumber)
933+
value, _ := syncMap.LoadOrStore(key, &sync.Once{})
934+
var ok bool
935+
once, ok = value.(*sync.Once)
936+
if !ok {
937+
return fmt.Errorf("unexpected type in sync map for key %s", key)
938+
}
939+
} else {
940+
once = &sync.Once{}
1031941
}
1032942

1033-
// Best-effort post-create reconciliation to collapse duplicates created by
1034-
// concurrent processors handling the same event.
1035-
matchedComments, listErr := v.listCommentsByMarker(ctx, event, updateMarker, "post_create_list", trace)
1036-
if listErr != nil {
1037-
return nil
1038-
}
1039-
if len(matchedComments) > 1 {
1040-
v.debugCommentPhase(event, trace, "duplicate_detected", "matched_count", len(matchedComments))
1041-
}
1042-
return v.ensureSingleMarkerComment(ctx, event, matchedComments, commit, trace)
943+
once.Do(func() {
944+
_, _, err = wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) {
945+
return v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{
946+
Body: github.Ptr(commit),
947+
})
948+
})
949+
})
950+
return err
1043951
}

0 commit comments

Comments
 (0)