Skip to content

Commit a6d0b01

Browse files
chmouelchmouel
committed
fix: Improve handling of duplicate GitHub comments
The logic for updating and creating comments on GitHub pull requests was updated to better handle scenarios with duplicate comments. Previously, the system might create multiple identical comments or fail to correctly update an existing one. This change refactored the comment handling to first retrieve all comments matching a marker. If duplicates are found, it now prioritizes one comment to update and deletes the others. Additionally, a jittered retry mechanism was introduced when creating new comments to reduce the chance of race conditions where multiple processors might simultaneously create duplicate comments. The test suite was also updated to cover these deduplication scenarios. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
1 parent dd6f248 commit a6d0b01

4 files changed

Lines changed: 178 additions & 62 deletions

File tree

‎pkg/pipelineascode/match.go‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -412,15 +412,11 @@ func (p *PacRun) resolveTargetNamespaceRepo(ctx context.Context, fallbackRepo *v
412412

413413
targetRepo, err := matcher.MatchEventURLRepo(ctx, p.run, p.event, targetNS)
414414
if err != nil {
415-
if p.logger != nil {
416-
p.logger.Warnf("resolveTargetNamespaceRepo: failed to lookup target namespace=%s for pipelinerun=%s: %v", targetNS, pipelineRunIdentifier(pr), err)
417-
}
415+
p.logger.Warnf("resolveTargetNamespaceRepo: failed to lookup target namespace=%s for pipelinerun=%s: %v", targetNS, pipelineRunIdentifier(pr), err)
418416
return fallbackRepo
419417
}
420418
if targetRepo == nil {
421-
if p.logger != nil {
422-
p.logger.Warnf("resolveTargetNamespaceRepo: no repository found in target namespace=%s for pipelinerun=%s", targetNS, pipelineRunIdentifier(pr))
423-
}
419+
p.logger.Warnf("resolveTargetNamespaceRepo: no repository found in target namespace=%s for pipelinerun=%s", targetNS, pipelineRunIdentifier(pr))
424420
return fallbackRepo
425421
}
426422

‎pkg/provider/github/github.go‎

Lines changed: 70 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -750,8 +750,7 @@ func (v *Provider) GetTemplate(commentType provider.CommentType) string {
750750
return provider.GetHTMLTemplate(commentType)
751751
}
752752

753-
// listAndFindComment lists comments on a PR and returns the first one matching the marker.
754-
func (v *Provider) listAndFindComment(ctx context.Context, event *info.Event, marker string) (*github.IssueComment, error) {
753+
func (v *Provider) listCommentsByMarker(ctx context.Context, event *info.Event, marker string) ([]*github.IssueComment, error) {
755754
comments, _, err := wrapAPI(v, "list_comments", func() ([]*github.IssueComment, *github.Response, error) {
756755
return v.Client().Issues.ListComments(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueListCommentsOptions{
757756
ListOptions: github.ListOptions{
@@ -765,12 +764,57 @@ func (v *Provider) listAndFindComment(ctx context.Context, event *info.Event, ma
765764
}
766765

767766
re := regexp.MustCompile(regexp.QuoteMeta(marker))
767+
matchedComments := make([]*github.IssueComment, 0, len(comments))
768768
for _, comment := range comments {
769769
if re.MatchString(comment.GetBody()) {
770-
return comment, nil
770+
matchedComments = append(matchedComments, comment)
771771
}
772772
}
773-
return nil, nil
773+
return matchedComments, nil
774+
}
775+
776+
func (v *Provider) ensureSingleMarkerComment(
777+
ctx context.Context,
778+
event *info.Event,
779+
comments []*github.IssueComment,
780+
commit string,
781+
) error {
782+
if len(comments) == 0 {
783+
return nil
784+
}
785+
786+
primaryComment := comments[0]
787+
for _, comment := range comments {
788+
if comment.GetBody() == commit {
789+
primaryComment = comment
790+
break
791+
}
792+
}
793+
794+
if primaryComment.GetBody() != commit {
795+
if _, _, err := wrapAPI(v, "edit_comment", func() (*github.IssueComment, *github.Response, error) {
796+
return v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, primaryComment.GetID(), &github.IssueComment{
797+
Body: github.Ptr(commit),
798+
})
799+
}); err != nil {
800+
return err
801+
}
802+
}
803+
804+
// Best-effort cleanup to collapse duplicates into a single canonical marker comment.
805+
for _, comment := range comments {
806+
if comment.GetID() == primaryComment.GetID() {
807+
continue
808+
}
809+
if _, _, err := wrapAPI(v, "delete_comment", func() (struct{}, *github.Response, error) {
810+
resp, err := v.Client().Issues.DeleteComment(ctx, event.Organization, event.Repository, comment.GetID())
811+
return struct{}{}, resp, err
812+
}); err != nil && v.Logger != nil {
813+
v.Logger.Warnf("failed to delete duplicate comment %d on %s/%s#%d: %v",
814+
comment.GetID(), event.Organization, event.Repository, event.PullRequestNumber, err)
815+
}
816+
}
817+
return nil
774818
}
775819

776820
// CreateComment creates a comment on a Pull Request.
@@ -784,34 +828,15 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
784828
}
785829

786830
if updateMarker != "" {
787-
existingComment, err := v.listAndFindComment(ctx, event, updateMarker)
831+
existingComments, err := v.listCommentsByMarker(ctx, event, updateMarker)
788832
if err != nil {
789833
return err
790834
}
791835

792-
if existingComment != nil {
793-
// No edit required if the comment is exactly the same.
794-
if existingComment.GetBody() == commit {
795-
return nil
796-
}
797-
if _, _, err := wrapAPI(v, "edit_comment", func() (*github.IssueComment, *github.Response, error) {
798-
return v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, existingComment.GetID(), &github.IssueComment{
799-
Body: github.Ptr(commit),
800-
})
801-
}); err != nil {
802-
return err
803-
}
804-
return nil
836+
if len(existingComments) > 0 {
837+
return v.ensureSingleMarkerComment(ctx, event, existingComments, commit)
805838
}
806839

807-
// HACK: Workaround for duplicate comment creation issue.
808-
// In E2E tests, we occasionally see two identical comments created on a PR when
809-
// there should only be one. The root cause is unclear, despite only one
810-
// create_comment API call being logged, two comments appear on the PR.
811-
//
812-
// This workaround adds a random sleep (0-500ms) before re-checking for existing
813-
// comments. This reduces the window where parallel processes might both
814-
// see no existing comment and both decide to create one.
815840
//nolint:gosec // No need for crypto/rand here, just reducing timing window
816841
jitter := time.Duration(rand.Intn(500)) * time.Millisecond
817842
timer := time.NewTimer(jitter)
@@ -823,13 +848,13 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
823848
case <-timer.C:
824849
}
825850

826-
// Re-check if a comment exists now
827-
existingComment, err = v.listAndFindComment(ctx, event, updateMarker)
851+
// Re-check after jitter in case another processor already created the marker comment.
852+
existingComments, err = v.listCommentsByMarker(ctx, event, updateMarker)
828853
if err != nil {
829854
return err
830855
}
831-
if existingComment != nil {
832-
return nil
856+
if len(existingComments) > 0 {
857+
return v.ensureSingleMarkerComment(ctx, event, existingComments, commit)
833858
}
834859
}
835860

@@ -838,5 +863,19 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
838863
Body: github.Ptr(commit),
839864
})
840865
})
841-
return err
866+
if err != nil {
867+
return err
868+
}
869+
870+
if updateMarker == "" {
871+
return nil
872+
}
873+
874+
// Best-effort post-create reconciliation to collapse duplicates created by
875+
// concurrent processors handling the same event.
876+
matchedComments, listErr := v.listCommentsByMarker(ctx, event, updateMarker)
877+
if listErr != nil {
878+
return nil
879+
}
880+
return v.ensureSingleMarkerComment(ctx, event, matchedComments, commit)
842881
}

‎pkg/provider/github/github_test.go‎

Lines changed: 101 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1430,12 +1430,13 @@ func TestIsHeadCommitOfBranch(t *testing.T) {
14301430

14311431
func TestCreateComment(t *testing.T) {
14321432
tests := []struct {
1433-
name string
1434-
event *info.Event
1435-
updateMarker string
1436-
mockResponses map[string]func(rw http.ResponseWriter, _ *http.Request)
1437-
wantErr string
1438-
clientNil bool
1433+
name string
1434+
event *info.Event
1435+
updateMarker string
1436+
commentBody string
1437+
clientNil bool
1438+
wantErr string
1439+
setup func(t *testing.T, mux *http.ServeMux) func(t *testing.T)
14391440
}{
14401441
{
14411442
name: "nil client error",
@@ -1452,43 +1453,114 @@ func TestCreateComment(t *testing.T) {
14521453
name: "create new comment",
14531454
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
14541455
updateMarker: "",
1455-
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
1456-
"/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) {
1456+
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
1457+
t.Helper()
1458+
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
14571459
assert.Equal(t, r.Method, http.MethodPost)
14581460
rw.WriteHeader(http.StatusCreated)
1459-
},
1461+
})
1462+
return nil
14601463
},
14611464
},
14621465
{
14631466
name: "update existing comment",
14641467
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
14651468
updateMarker: "MARKER",
1466-
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
1467-
"/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) {
1469+
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
1470+
t.Helper()
1471+
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
14681472
if r.Method == http.MethodGet {
14691473
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`)
14701474
return
14711475
}
1472-
},
1473-
"/repos/org/repo/issues/comments/555": func(rw http.ResponseWriter, r *http.Request) {
1476+
})
1477+
mux.HandleFunc("/repos/org/repo/issues/comments/555", func(rw http.ResponseWriter, r *http.Request) {
14741478
assert.Equal(t, r.Method, http.MethodPatch)
14751479
rw.WriteHeader(http.StatusOK)
1476-
},
1480+
})
1481+
return nil
14771482
},
14781483
},
14791484
{
14801485
name: "no matching comment creates new",
14811486
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
14821487
updateMarker: "MARKER",
1483-
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
1484-
"/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) {
1488+
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
1489+
t.Helper()
1490+
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
14851491
if r.Method == http.MethodGet {
14861492
fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`)
14871493
return
14881494
}
14891495
assert.Equal(t, r.Method, http.MethodPost)
14901496
rw.WriteHeader(http.StatusCreated)
1491-
},
1497+
})
1498+
return nil
1499+
},
1500+
},
1501+
{
1502+
name: "deduplicates existing marker comments",
1503+
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
1504+
updateMarker: "MARKER",
1505+
commentBody: "new body MARKER",
1506+
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
1507+
t.Helper()
1508+
editedPrimary := false
1509+
deletedDuplicate := false
1510+
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
1511+
assert.Equal(t, r.Method, http.MethodGet)
1512+
fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old"}, {"id": 222, "body": "MARKER old"}]`)
1513+
})
1514+
mux.HandleFunc("/repos/org/repo/issues/comments/111", func(rw http.ResponseWriter, r *http.Request) {
1515+
assert.Equal(t, r.Method, http.MethodPatch)
1516+
editedPrimary = true
1517+
rw.WriteHeader(http.StatusOK)
1518+
})
1519+
mux.HandleFunc("/repos/org/repo/issues/comments/222", func(rw http.ResponseWriter, r *http.Request) {
1520+
assert.Equal(t, r.Method, http.MethodDelete)
1521+
deletedDuplicate = true
1522+
rw.WriteHeader(http.StatusNoContent)
1523+
})
1524+
return func(t *testing.T) {
1525+
t.Helper()
1526+
assert.Assert(t, editedPrimary)
1527+
assert.Assert(t, deletedDuplicate)
1528+
}
1529+
},
1530+
},
1531+
{
1532+
name: "deduplicates post-create marker comments",
1533+
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
1534+
updateMarker: "MARKER",
1535+
commentBody: "body MARKER",
1536+
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
1537+
t.Helper()
1538+
listCalls := 0
1539+
deletedDuplicate := false
1540+
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
1541+
switch r.Method {
1542+
case http.MethodGet:
1543+
listCalls++
1544+
if listCalls <= 2 {
1545+
fmt.Fprint(rw, `[]`)
1546+
return
1547+
}
1548+
fmt.Fprint(rw, `[{"id": 111, "body": "body MARKER"}, {"id": 222, "body": "body MARKER"}]`)
1549+
case http.MethodPost:
1550+
rw.WriteHeader(http.StatusCreated)
1551+
default:
1552+
t.Fatalf("unexpected method: %s", r.Method)
1553+
}
1554+
})
1555+
mux.HandleFunc("/repos/org/repo/issues/comments/222", func(rw http.ResponseWriter, r *http.Request) {
1556+
assert.Equal(t, r.Method, http.MethodDelete)
1557+
deletedDuplicate = true
1558+
rw.WriteHeader(http.StatusNoContent)
1559+
})
1560+
return func(t *testing.T) {
1561+
t.Helper()
1562+
assert.Assert(t, deletedDuplicate)
1563+
}
14921564
},
14931565
},
14941566
}
@@ -1498,25 +1570,34 @@ func TestCreateComment(t *testing.T) {
14981570
ctx, _ := rtesting.SetupFakeContext(t)
14991571

15001572
var provider *Provider
1573+
var postAssert func(t *testing.T)
15011574
if !tt.clientNil {
15021575
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
15031576
defer teardown()
15041577
provider = &Provider{ghClient: fakeclient}
15051578

1506-
for pattern, handler := range tt.mockResponses {
1507-
mux.HandleFunc(pattern, handler)
1579+
if tt.setup != nil {
1580+
postAssert = tt.setup(t, mux)
15081581
}
15091582
} else {
15101583
provider = &Provider{} // nil client
15111584
}
15121585

1513-
err := provider.CreateComment(ctx, tt.event, "comment body", tt.updateMarker)
1586+
body := tt.commentBody
1587+
if body == "" {
1588+
body = "comment body"
1589+
}
1590+
1591+
err := provider.CreateComment(ctx, tt.event, body, tt.updateMarker)
15141592

15151593
if tt.wantErr != "" {
15161594
assert.ErrorContains(t, err, tt.wantErr)
15171595
return
15181596
}
15191597
assert.NilError(t, err)
1598+
if postAssert != nil {
1599+
postAssert(t)
1600+
}
15201601
})
15211602
}
15221603
}

‎test/pkg/github/setup.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,11 @@ func Setup(ctx context.Context, onSecondController, viaDirectWebhook bool) (cont
6565
return ctx, nil, options.E2E{}, github.New(), err
6666
}
6767
run.Info.Controller = info.GetControllerInfoFromEnvOrDefault()
68+
ctxWithInfo, err := cctx.GetControllerCtxInfo(ctx, run)
69+
if err != nil {
70+
return ctx, nil, options.E2E{}, github.New(), err
71+
}
72+
ctx = ctxWithInfo
6873
e2eoptions := options.E2E{Organization: split[0], Repo: split[1], DirectWebhook: viaDirectWebhook, ControllerURL: controllerURL}
6974
gprovider := github.New()
7075
gprovider.Run = run
@@ -73,11 +78,6 @@ func Setup(ctx context.Context, onSecondController, viaDirectWebhook bool) (cont
7378
if githubToken == "" && !viaDirectWebhook {
7479
var err error
7580

76-
ctx, err = cctx.GetControllerCtxInfo(ctx, run)
77-
if err != nil {
78-
return ctx, nil, options.E2E{}, github.New(), err
79-
}
80-
8181
envGithubRepoInstallationID, err := setup.GetRequiredEnv("TEST_GITHUB_REPO_INSTALLATION_ID")
8282
if err != nil {
8383
return ctx, nil, options.E2E{}, github.New(), err

0 commit comments

Comments
 (0)