fix(usecase): report a conflict when every issue comment save conflicts instead of publishing IssueCommented

commit: a4f8ebdaf2ab4504dcb661d50c8ea0d2c4794613
author: brice · 3d ago · 2026-10-19 06:06 UTC
parent: e3b6724
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +26 -3

M internal/usecase/issues.go +8 -3


@@ -186,6 +186,7 @@ func (i *Issues) AddComment(ctx context.Context, p domain.Principal, name domain
186	186		return domain.IssueComment{}, err
187	187		}
188	188		var c domain.IssueComment
	189	+	saved := false
189	190		for range maxWriteRetries {
190	191		if issue.State == domain.IssueStatePending {
191	192		return domain.IssueComment{}, fmt.Errorf("%w: #%d is %s", domain.ErrPreconditionFailed, number, issue.State)

@@ -196,15 +197,19 @@ func (i *Issues) AddComment(ctx context.Context, p domain.Principal, name domain
196	197		c = domain.IssueComment{ID: issue.CommentSeq + 1, Author: authorOf(p), Text: body, CreatedAt: i.clock.Now().UTC()}
197	198		issue.CommentSeq, issue.UpdatedAt = c.ID, c.CreatedAt
198	199		err = i.store.SaveComment(ctx, issue, c)
199		-	if !errors.Is(err, domain.ErrConflict) {
	200	+	if err == nil {
	201	+	saved = true
200	202		break
201	203		}
	204	+	if !errors.Is(err, domain.ErrConflict) {
	205	+	return domain.IssueComment{}, fmt.Errorf("save comment on #%d of %q: %w", number, name, err)
	206	+	}
202	207		if issue, err = i.store.Get(ctx, name, number); err != nil {
203	208		return domain.IssueComment{}, err
204	209		}
205	210		}
206		-	if err != nil {
207		-	return domain.IssueComment{}, fmt.Errorf("save comment on #%d of %q: %w", number, name, err)
	211	+	if !saved {
	212	+	return domain.IssueComment{}, fmt.Errorf("save comment on #%d of %q: %w", number, name, domain.ErrConflict)
208	213		}
209	214		log := zerowrap.FromCtx(ctx)
210	215		log.Info().Str("repo", name.String()).Int64("number", number).Int64("comment", c.ID).

 		return domain.IssueComment{}, err
 	}
 	var c domain.IssueComment
+	saved := false
 	for range maxWriteRetries {
 		if issue.State == domain.IssueStatePending {
 			return domain.IssueComment{}, fmt.Errorf("%w: #%d is %s", domain.ErrPreconditionFailed, number, issue.State)
 		c = domain.IssueComment{ID: issue.CommentSeq + 1, Author: authorOf(p), Text: body, CreatedAt: i.clock.Now().UTC()}
 		issue.CommentSeq, issue.UpdatedAt = c.ID, c.CreatedAt
 		err = i.store.SaveComment(ctx, issue, c)
-		if !errors.Is(err, domain.ErrConflict) {
+		if err == nil {
+			saved = true
 			break
 		}
+		if !errors.Is(err, domain.ErrConflict) {
+			return domain.IssueComment{}, fmt.Errorf("save comment on #%d of %q: %w", number, name, err)
+		}
 		if issue, err = i.store.Get(ctx, name, number); err != nil {
 			return domain.IssueComment{}, err
 		}
 	}
-	if err != nil {
+	if !saved {
-		return domain.IssueComment{}, fmt.Errorf("save comment on #%d of %q: %w", number, name, err)
+		return domain.IssueComment{}, fmt.Errorf("save comment on #%d of %q: %w", number, name, domain.ErrConflict)
 	}
 	log := zerowrap.FromCtx(ctx)
 	log.Info().Str("repo", name.String()).Int64("number", number).Int64("comment", c.ID).

M internal/usecase/issues_test.go +18 -0


@@ -128,6 +128,15 @@ func TestIssueCreateAndTransitions(t *testing.T) {
128	128
129	129		open := domain.Issue{Repo: name(t, "proj"), Number: 3, State: domain.IssueStateOpen, Version: 1}
130	130
	131	+	t.Run("close with every update conflicting is an error and publishes nothing", func(t *testing.T) {
	132	+	t.Parallel()
	133	+	u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)
	134	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(3)).Return(open, nil)
	135	+	u.store.EXPECT().Update(mock.Anything, mock.Anything).Return(domain.ErrConflict)
	136	+	_, err := u.issues.Close(ctx, p, u.repo.Name, 3)
	137	+	assert.ErrorIs(t, err, domain.ErrConflict)
	138	+	})
	139	+
131	140		t.Run("close retries a conflict then publishes", func(t *testing.T) {
132	141		t.Parallel()
133	142		u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)

@@ -181,6 +190,15 @@ func TestIssueAddComment(t *testing.T) {
181	190		p := issuePrincipal(domain.ScopeIssueWrite)
182	191		issue := domain.Issue{Repo: name(t, "proj"), Number: 3, State: domain.IssueStateClosed, CommentSeq: 4, Version: 1}
183	192
	193	+	t.Run("every save conflicting is an error and publishes nothing", func(t *testing.T) {
	194	+	t.Parallel()
	195	+	u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)
	196	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(3)).Return(issue, nil)
	197	+	u.store.EXPECT().SaveComment(mock.Anything, mock.Anything, mock.Anything).Return(domain.ErrConflict)
	198	+	_, err := u.issues.AddComment(ctx, p, u.repo.Name, 3, "hi") // the bus mock fails on any Publish
	199	+	assert.ErrorIs(t, err, domain.ErrConflict)
	200	+	})
	201	+
184	202		t.Run("on a closed issue, with a retry", func(t *testing.T) {
185	203		t.Parallel()
186	204		u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)


@@ -128,6 +128,15 @@ func TestIssueCreateAndTransitions(t *testing.T) {
128		128
129	open := domain.Issue{Repo: name(t, "proj"), Number: 3, State: domain.IssueStateOpen, Version: 1}	129		open := domain.Issue{Repo: name(t, "proj"), Number: 3, State: domain.IssueStateOpen, Version: 1}
130		130
		131	+	t.Run("close with every update conflicting is an error and publishes nothing", func(t *testing.T) {
		132	+	t.Parallel()
		133	+	u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)
		134	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(3)).Return(open, nil)
		135	+	u.store.EXPECT().Update(mock.Anything, mock.Anything).Return(domain.ErrConflict)
		136	+	_, err := u.issues.Close(ctx, p, u.repo.Name, 3)
		137	+	assert.ErrorIs(t, err, domain.ErrConflict)
		138	+	})
		139	+
131	t.Run("close retries a conflict then publishes", func(t *testing.T) {	140		t.Run("close retries a conflict then publishes", func(t *testing.T) {
132	t.Parallel()	141		t.Parallel()
133	u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)	142		u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)

@@ -181,6 +190,15 @@ func TestIssueAddComment(t *testing.T) {
181	p := issuePrincipal(domain.ScopeIssueWrite)	190		p := issuePrincipal(domain.ScopeIssueWrite)
182	issue := domain.Issue{Repo: name(t, "proj"), Number: 3, State: domain.IssueStateClosed, CommentSeq: 4, Version: 1}	191		issue := domain.Issue{Repo: name(t, "proj"), Number: 3, State: domain.IssueStateClosed, CommentSeq: 4, Version: 1}
183		192
		193	+	t.Run("every save conflicting is an error and publishes nothing", func(t *testing.T) {
		194	+	t.Parallel()
		195	+	u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)
		196	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(3)).Return(issue, nil)
		197	+	u.store.EXPECT().SaveComment(mock.Anything, mock.Anything, mock.Anything).Return(domain.ErrConflict)
		198	+	_, err := u.issues.AddComment(ctx, p, u.repo.Name, 3, "hi") // the bus mock fails on any Publish
		199	+	assert.ErrorIs(t, err, domain.ErrConflict)
		200	+	})
		201	+
184	t.Run("on a closed issue, with a retry", func(t *testing.T) {	202		t.Run("on a closed issue, with a retry", func(t *testing.T) {
185	t.Parallel()	203		t.Parallel()
186	u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)	204		u := newIssueUnit(t, domain.VisibilityPublic, domain.IssuesOwner)