fix(mr): recheck the reply target when a note save is retried

commit: aa64b62ba28ff6b206beae157e940b28f30d8871
author: brice · 3d ago · 2026-10-19 05:34 UTC
parent: b31a354
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +41 -12

M internal/usecase/mergerequests.go +28 -12


@@ -594,20 +594,11 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
594	594		return domain.Comment{}, err
595	595		}
596	596		if params.ReplyTo != 0 {
597		-	root, err := m.store.GetComment(ctx, name, number, params.ReplyTo)
598		-	switch {
599		-	case errors.Is(err, domain.ErrNotFound):
600		-	v.Add("reply_to", fmt.Sprintf("no note ^%d", params.ReplyTo))
601		-	case err != nil:
602		-	return domain.Comment{}, err
603		-	case root.IsReply() \|\| root.Deleted:
604		-	v.Add("reply_to", "must name a non-deleted root note")
605		-	default:
606		-	params.Path, params.Line, params.StartLine, params.Side = root.Path, root.Line, root.StartLine, root.Side
607		-	}
608		-	if err := v.Err(); err != nil {
	597	+	root, err := m.replyRoot(ctx, name, number, params.ReplyTo)
	598	+	if err != nil {
609	599		return domain.Comment{}, err
610	600		}
	601	+	params.Path, params.Line, params.StartLine, params.Side = root.Path, root.Line, root.StartLine, root.Side
611	602		}
612	603		if params.Verdict != domain.VerdictNone {
613	604		if err := scopeAccess(p, repo, domain.ScopeMRReview); err != nil {

@@ -650,6 +641,12 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
650	641		if !fresh.IsOpen() {
651	642		return domain.Comment{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, fresh.State)
652	643		}
	644	+	if params.ReplyTo != 0 {
	645	+	// The root may have been deleted by the write that got in between.
	646	+	if _, rerr := m.replyRoot(ctx, name, number, params.ReplyTo); rerr != nil {
	647	+	return domain.Comment{}, rerr
	648	+	}
	649	+	}
653	650		fresh.HeadID = mr.HeadID
654	651		mr = fresh
655	652		}

@@ -1241,3 +1238,22 @@ func (m *MergeRequests) changeComment(ctx context.Context, p domain.Principal, n
1241	1238		}
1242	1239		return domain.Comment{}, fmt.Errorf("save note %d of !%d in %q: %w", commentID, number, name, domain.ErrConflict)
1243	1240		}
	1241	+
	1242	+	// replyRoot returns the note a reply targets, or a violation when it is
	1243	+	// missing, itself a reply, or deleted.
	1244	+	func (m *MergeRequests) replyRoot(ctx context.Context, name domain.RepoName, number, id int64) (domain.Comment, error) {
	1245	+	root, err := m.store.GetComment(ctx, name, number, id)
	1246	+	var v domain.Violations
	1247	+	switch {
	1248	+	case errors.Is(err, domain.ErrNotFound):
	1249	+	v.Add("reply_to", fmt.Sprintf("no note ^%d", id))
	1250	+	case err != nil:
	1251	+	return domain.Comment{}, err
	1252	+	case root.IsReply() \|\| root.Deleted:
	1253	+	v.Add("reply_to", "must name a non-deleted root note")
	1254	+	}
	1255	+	if err := v.Err(); err != nil {
	1256	+	return domain.Comment{}, err
	1257	+	}
	1258	+	return root, nil
	1259	+	}

 		return domain.Comment{}, err
 	}
 	if params.ReplyTo != 0 {
-		root, err := m.store.GetComment(ctx, name, number, params.ReplyTo)
+		root, err := m.replyRoot(ctx, name, number, params.ReplyTo)
-		switch {
+		if err != nil {
-		case errors.Is(err, domain.ErrNotFound):
-			v.Add("reply_to", fmt.Sprintf("no note ^%d", params.ReplyTo))
-		case err != nil:
-			return domain.Comment{}, err
-		case root.IsReply() || root.Deleted:
-			v.Add("reply_to", "must name a non-deleted root note")
-		default:
-			params.Path, params.Line, params.StartLine, params.Side = root.Path, root.Line, root.StartLine, root.Side
-		}
-		if err := v.Err(); err != nil {
 			return domain.Comment{}, err
 		}
+		params.Path, params.Line, params.StartLine, params.Side = root.Path, root.Line, root.StartLine, root.Side
 	}
 	if params.Verdict != domain.VerdictNone {
 		if err := scopeAccess(p, repo, domain.ScopeMRReview); err != nil {
 		if !fresh.IsOpen() {
 			return domain.Comment{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, fresh.State)
 		}
+		if params.ReplyTo != 0 {
+			// The root may have been deleted by the write that got in between.
+			if _, rerr := m.replyRoot(ctx, name, number, params.ReplyTo); rerr != nil {
+				return domain.Comment{}, rerr
+			}
+		}
 		fresh.HeadID = mr.HeadID
 		mr = fresh
 	}
 	}
 	return domain.Comment{}, fmt.Errorf("save note %d of !%d in %q: %w", commentID, number, name, domain.ErrConflict)
 }
+// replyRoot returns the note a reply targets, or a violation when it is
+// missing, itself a reply, or deleted.
+func (m *MergeRequests) replyRoot(ctx context.Context, name domain.RepoName, number, id int64) (domain.Comment, error) {
+	root, err := m.store.GetComment(ctx, name, number, id)
+	var v domain.Violations
+	switch {
+	case errors.Is(err, domain.ErrNotFound):
+		v.Add("reply_to", fmt.Sprintf("no note ^%d", id))
+	case err != nil:
+		return domain.Comment{}, err
+	case root.IsReply() || root.Deleted:
+		v.Add("reply_to", "must name a non-deleted root note")
+	}
+	if err := v.Err(); err != nil {
+		return domain.Comment{}, err
+	}
+	return root, nil
+}

M internal/usecase/mergerequests_test.go +13 -0


@@ -152,6 +152,19 @@ func TestNoteMutationRetries(t *testing.T) {
152	152		}
153	153		}
154	154
	155	+	// A reply whose root is deleted while the save is retried is rejected.
	156	+	func TestAddNoteRechecksReplyRootOnConflict(t *testing.T) {
	157	+	u := newMRUnit(t)
	158	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	159	+	u.git.EXPECT().BranchHead(mock.Anything, mock.Anything, mock.Anything).Return(domain.ObjectID{}, domain.ErrNotFound)
	160	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(domain.MergeRequest{State: domain.MRStateOpen}, nil).Twice()
	161	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(domain.Comment{ID: 1}, nil).Once()
	162	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(domain.Comment{ID: 1, Deleted: true}, nil).Once()
	163	+	u.store.EXPECT().SaveComment(mock.Anything, mock.Anything, mock.Anything).Return(domain.ErrConflict).Once()
	164	+	_, err := u.mrs.AddComment(context.Background(), domain.Root(), u.repo.Name, 1, ports.AddCommentParams{ReplyTo: 1, Text: "x"})
	165	+	require.ErrorIs(t, err, domain.ErrInvalid)
	166	+	}
	167	+
155	168		func TestAddNoteValidation(t *testing.T) {
156	169		for _, tc := range []struct {
157	170		name string


@@ -152,6 +152,19 @@ func TestNoteMutationRetries(t *testing.T) {
152	}	152		}
153	}	153		}
154		154
		155	+	// A reply whose root is deleted while the save is retried is rejected.
		156	+	func TestAddNoteRechecksReplyRootOnConflict(t *testing.T) {
		157	+	u := newMRUnit(t)
		158	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
		159	+	u.git.EXPECT().BranchHead(mock.Anything, mock.Anything, mock.Anything).Return(domain.ObjectID{}, domain.ErrNotFound)
		160	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(domain.MergeRequest{State: domain.MRStateOpen}, nil).Twice()
		161	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(domain.Comment{ID: 1}, nil).Once()
		162	+	u.store.EXPECT().GetComment(mock.Anything, u.repo.Name, int64(1), int64(1)).Return(domain.Comment{ID: 1, Deleted: true}, nil).Once()
		163	+	u.store.EXPECT().SaveComment(mock.Anything, mock.Anything, mock.Anything).Return(domain.ErrConflict).Once()
		164	+	_, err := u.mrs.AddComment(context.Background(), domain.Root(), u.repo.Name, 1, ports.AddCommentParams{ReplyTo: 1, Text: "x"})
		165	+	require.ErrorIs(t, err, domain.ErrInvalid)
		166	+	}
		167	+
155	func TestAddNoteValidation(t *testing.T) {	168		func TestAddNoteValidation(t *testing.T) {
156	for _, tc := range []struct {	169		for _, tc := range []struct {
157	name string	170		name string