fix(app): return Git and settings read failures as errors

Only a missing branch is a field violation of source_branch or target_branch;
any other Git failure is returned so it is logged and masked, not shown to the
caller. Merge no longer ignores a failure to read the settings after taking the
lock: it is returned, and a repository deleted meanwhile is not found.

commit: e51c1e8a2050579438305ab619f280ce37952bb2
author: brice · 11d ago · 2026-10-10 17:46 UTC
parent: f878cf1
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +130 -9

M internal/app/mergerequests.go +20 -9


@@ -180,7 +180,10 @@ func (m *MergeRequests) Create(ctx context.Context, p domain.Principal, name dom
180	180		if err != nil {
181	181		v.AddErr("description", err)
182	182		}
183		-	source, target := m.checkBranches(ctx, name, params, &v)
	183	+	source, target, err := m.checkBranches(ctx, name, params, &v)
	184	+	if err != nil {
	185	+	return domain.MergeRequest{}, err
	186	+	}
184	187		if err := v.Err(); err != nil {
185	188		return domain.MergeRequest{}, err
186	189		}

@@ -224,7 +227,11 @@ func (m *MergeRequests) Create(ctx context.Context, p domain.Principal, name dom
224	227
225	228		// checkBranches validates the branches of a new request and returns them, with
226	229		// the default branch filled in for the target.
227		-	func (m MergeRequests) checkBranches(ctx context.Context, name domain.RepoName, params CreateMergeRequestParams, v domain.Violations) (source, target string) {
	230	+	//
	231	+	// Only a branch that does not exist is a violation. Any other failure to read
	232	+	// Git is returned as an error, so that it is logged and masked, not shown in
	233	+	// the field violations.
	234	+	func (m MergeRequests) checkBranches(ctx context.Context, name domain.RepoName, params CreateMergeRequestParams, v domain.Violations) (source, target string, err error) {
228	235		source, target = params.Source, params.Target
229	236		if source == "" {
230	237		v.Add("source_branch", "is required")

@@ -235,8 +242,8 @@ func (m *MergeRequests) checkBranches(ctx context.Context, name domain.RepoName,
235	242		if target == "" {
236	243		def, exists, err := m.repos.DefaultBranch(ctx, name)
237	244		switch {
238		-	case err != nil:
239		-	v.Add("target_branch", "cannot be read: "+err.Error())
	245	+	case err != nil: // includes domain.ErrNotFound: the repository is gone
	246	+	return "", "", fmt.Errorf("read the default branch of %q: %w", name, err)
240	247		case !exists:
241	248		v.Add("target_branch", fmt.Sprintf("is empty and the default branch %q does not exist", def))
242	249		default:

@@ -248,7 +255,7 @@ func (m *MergeRequests) checkBranches(ctx context.Context, name domain.RepoName,
248	255		}
249	256		if source != "" && source == target {
250	257		v.Add("target_branch", "must differ from source_branch")
251		-	return source, target
	258	+	return source, target, nil
252	259		}
253	260		for _, b := range []struct{ field, name string }{{"source_branch", source}, {"target_branch", target}} {
254	261		if b.name == "" {

@@ -258,10 +265,10 @@ func (m *MergeRequests) checkBranches(ctx context.Context, name domain.RepoName,
258	265		if errors.Is(err, domain.ErrNotFound) {
259	266		v.Add(b.field, fmt.Sprintf("branch %q does not exist", b.name))
260	267		} else if err != nil {
261		-	v.Add(b.field, "cannot be read: "+err.Error())
	268	+	return "", "", fmt.Errorf("read branch %q of %q: %w", b.name, name, err)
262	269		}
263	270		}
264		-	return source, target
	271	+	return source, target, nil
265	272		}
266	273
267	274		// ListMergeRequestsParams selects a page of merge requests.

@@ -737,8 +744,12 @@ func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name doma
737	744		return domain.MergeRequest{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State)
738	745		}
739	746		// The settings are read again under the lock: a change applies at once.
740		-	if current, err := m.meta.Get(ctx, name); err == nil {
741		-	repo = current
	747	+	repo, err = m.meta.Get(ctx, name)
	748	+	if err != nil {
	749	+	if errors.Is(err, domain.ErrNotFound) {
	750	+	return domain.MergeRequest{}, err // deleted meanwhile
	751	+	}
	752	+	return domain.MergeRequest{}, fmt.Errorf("get repository %q: %w", name, err)
742	753		}
743	754		settings := repo.Settings
744	755		if settings.IsZero() {

 	if err != nil {
 		v.AddErr("description", err)
 	}
-	source, target := m.checkBranches(ctx, name, params, &v)
+	source, target, err := m.checkBranches(ctx, name, params, &v)
+	if err != nil {
+		return domain.MergeRequest{}, err
+	}
 	if err := v.Err(); err != nil {
 		return domain.MergeRequest{}, err
 	}
 // checkBranches validates the branches of a new request and returns them, with
 // the default branch filled in for the target.
-func (m *MergeRequests) checkBranches(ctx context.Context, name domain.RepoName, params CreateMergeRequestParams, v *domain.Violations) (source, target string) {
+//
+// Only a branch that does not exist is a violation. Any other failure to read
+// Git is returned as an error, so that it is logged and masked, not shown in
+// the field violations.
+func (m *MergeRequests) checkBranches(ctx context.Context, name domain.RepoName, params CreateMergeRequestParams, v *domain.Violations) (source, target string, err error) {
 	source, target = params.Source, params.Target
 	if source == "" {
 		v.Add("source_branch", "is required")
 	if target == "" {
 		def, exists, err := m.repos.DefaultBranch(ctx, name)
 		switch {
-		case err != nil:
+		case err != nil: // includes domain.ErrNotFound: the repository is gone
-			v.Add("target_branch", "cannot be read: "+err.Error())
+			return "", "", fmt.Errorf("read the default branch of %q: %w", name, err)
 		case !exists:
 			v.Add("target_branch", fmt.Sprintf("is empty and the default branch %q does not exist", def))
 		default:
 	}
 	if source != "" && source == target {
 		v.Add("target_branch", "must differ from source_branch")
-		return source, target
+		return source, target, nil
 	}
 	for _, b := range []struct{ field, name string }{{"source_branch", source}, {"target_branch", target}} {
 		if b.name == "" {
 		if errors.Is(err, domain.ErrNotFound) {
 			v.Add(b.field, fmt.Sprintf("branch %q does not exist", b.name))
 		} else if err != nil {
-			v.Add(b.field, "cannot be read: "+err.Error())
+			return "", "", fmt.Errorf("read branch %q of %q: %w", b.name, name, err)
 		}
 	}
-	return source, target
+	return source, target, nil
 }
 // ListMergeRequestsParams selects a page of merge requests.
 		return domain.MergeRequest{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State)
 	}
 	// The settings are read again under the lock: a change applies at once.
-	if current, err := m.meta.Get(ctx, name); err == nil {
+	repo, err = m.meta.Get(ctx, name)
-		repo = current
+	if err != nil {
+		if errors.Is(err, domain.ErrNotFound) {
+			return domain.MergeRequest{}, err // deleted meanwhile
+		}
+		return domain.MergeRequest{}, fmt.Errorf("get repository %q: %w", name, err)
 	}
 	settings := repo.Settings
 	if settings.IsZero() {

A internal/app/mergerequests_test.go +110 -0

@@ -0,0 +1,110 @@
	1	+	// SPDX-License-Identifier: AGPL-3.0-or-later
	2	+
	3	+	package app_test
	4	+
	5	+	import (
	6	+	"context"
	7	+	"errors"
	8	+	"testing"
	9	+
	10	+	"github.com/stretchr/testify/assert"
	11	+	"github.com/stretchr/testify/mock"
	12	+	"github.com/stretchr/testify/require"
	13	+
	14	+	"bnema.dev/ermite/internal/app"
	15	+	"bnema.dev/ermite/internal/domain"
	16	+	"bnema.dev/ermite/internal/ports/mocks"
	17	+	)
	18	+
	19	+	type mrUnit struct {
	20	+	meta *mocks.RepoMetaStore
	21	+	store *mocks.MergeRequestStore
	22	+	repos *mocks.GitRepos
	23	+	git *mocks.MergeGit
	24	+	mrs *app.MergeRequests
	25	+	repo domain.Repo
	26	+	}
	27	+
	28	+	func newMRUnit(t testing.T) mrUnit {
	29	+	t.Helper()
	30	+	u := &mrUnit{
	31	+	meta: mocks.NewRepoMetaStore(t),
	32	+	store: mocks.NewMergeRequestStore(t),
	33	+	repos: mocks.NewGitRepos(t),
	34	+	git: mocks.NewMergeGit(t),
	35	+	repo: domain.Repo{Name: name(t, "proj"), Visibility: domain.VisibilityPrivate, Settings: domain.DefaultRepoSettings()},
	36	+	}
	37	+	clock := mocks.NewClock(t)
	38	+	clock.EXPECT().Now().Return(now).Maybe()
	39	+	u.mrs = app.NewMergeRequests(app.MergeRequestsConfig{
	40	+	Meta: u.meta, Store: u.store, Repos: u.repos, Git: u.git,
	41	+	Signer: mocks.NewCommitSigner(t), Bus: mocks.NewEventBus(t), Clock: clock,
	42	+	})
	43	+	return u
	44	+	}
	45	+
	46	+	// GO-004: a failure to read Git is an error for the log, not a field violation
	47	+	// that would show its text to the caller.
	48	+	func TestCreateReturnsGitFailuresAsErrors(t *testing.T) {
	49	+	boom := errors.New("fatal: /srv/ermite/repos/proj.git: permission denied")
	50	+	params := app.CreateMergeRequestParams{Title: "t", Source: "feat", Target: "main"}
	51	+
	52	+	t.Run("reading a branch", func(t *testing.T) {
	53	+	u := newMRUnit(t)
	54	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	55	+	u.git.EXPECT().BranchHead(mock.Anything, u.repo.Name, "feat").Return(domain.ObjectID{}, boom)
	56	+	_, err := u.mrs.Create(context.Background(), domain.Root(), u.repo.Name, params)
	57	+	require.ErrorIs(t, err, boom)
	58	+	var fe *domain.InvalidFieldsError
	59	+	assert.False(t, errors.As(err, &fe), "not a field violation: %v", err)
	60	+	assert.NotErrorIs(t, err, domain.ErrInvalid)
	61	+	})
	62	+
	63	+	t.Run("reading the default branch", func(t *testing.T) {
	64	+	u := newMRUnit(t)
	65	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	66	+	u.repos.EXPECT().DefaultBranch(mock.Anything, u.repo.Name).Return("", false, boom)
	67	+	_, err := u.mrs.Create(context.Background(), domain.Root(), u.repo.Name, app.CreateMergeRequestParams{Title: "t", Source: "feat"})
	68	+	require.ErrorIs(t, err, boom)
	69	+	var fe *domain.InvalidFieldsError
	70	+	assert.False(t, errors.As(err, &fe), "not a field violation: %v", err)
	71	+	})
	72	+
	73	+	t.Run("a branch that does not exist stays a violation", func(t *testing.T) {
	74	+	u := newMRUnit(t)
	75	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	76	+	u.git.EXPECT().BranchHead(mock.Anything, u.repo.Name, "feat").Return(domain.ObjectID{}, domain.ErrNotFound)
	77	+	u.git.EXPECT().BranchHead(mock.Anything, u.repo.Name, "main").Return(someID(t), nil)
	78	+	_, err := u.mrs.Create(context.Background(), domain.Root(), u.repo.Name, params)
	79	+	assert.Contains(t, fieldsOf(t, err), "source_branch")
	80	+	})
	81	+	}
	82	+
	83	+	func someID(t *testing.T) domain.ObjectID {
	84	+	t.Helper()
	85	+	return oidFrom(t, oidA)
	86	+	}
	87	+
	88	+	// GO-003: the settings are read again under the merge lock; a failure of that
	89	+	// read is returned, and a repository deleted meanwhile is not found.
	90	+	func TestMergeReturnsTheFailureToReadTheSettings(t *testing.T) {
	91	+	boom := errors.New("kv: closed")
	92	+	for label, tc := range map[string]struct{ err, want error }{
	93	+	"read failure": {boom, boom},
	94	+	"deleted meanwhile": {domain.ErrNotFound, domain.ErrNotFound},
	95	+	} {
	96	+	t.Run(label, func(t *testing.T) {
	97	+	u := newMRUnit(t)
	98	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil).Once()
	99	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(domain.Repo{}, tc.err).Once()
	100	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(domain.MergeRequest{
	101	+	Repo: u.repo.Name, Number: 1, State: domain.MRStateOpen, Source: "feat", Target: "main",
	102	+	}, nil)
	103	+	_, err := u.mrs.Merge(context.Background(), domain.Root(), u.repo.Name, 1, app.MergeParams{HeadID: someID(t)})
	104	+	require.ErrorIs(t, err, tc.want)
	105	+	if tc.want == boom {
	106	+	assert.NotErrorIs(t, err, domain.ErrNotFound)
	107	+	}
	108	+	})
	109	+	}
	110	+	}

@@ -0,0 +1,110 @@
	1	+	// SPDX-License-Identifier: AGPL-3.0-or-later
	2	+
	3	+	package app_test
	4	+
	5	+	import (
	6	+	"context"
	7	+	"errors"
	8	+	"testing"
	9	+
	10	+	"github.com/stretchr/testify/assert"
	11	+	"github.com/stretchr/testify/mock"
	12	+	"github.com/stretchr/testify/require"
	13	+
	14	+	"bnema.dev/ermite/internal/app"
	15	+	"bnema.dev/ermite/internal/domain"
	16	+	"bnema.dev/ermite/internal/ports/mocks"
	17	+	)
	18	+
	19	+	type mrUnit struct {
	20	+	meta *mocks.RepoMetaStore
	21	+	store *mocks.MergeRequestStore
	22	+	repos *mocks.GitRepos
	23	+	git *mocks.MergeGit
	24	+	mrs *app.MergeRequests
	25	+	repo domain.Repo
	26	+	}
	27	+
	28	+	func newMRUnit(t testing.T) mrUnit {
	29	+	t.Helper()
	30	+	u := &mrUnit{
	31	+	meta: mocks.NewRepoMetaStore(t),
	32	+	store: mocks.NewMergeRequestStore(t),
	33	+	repos: mocks.NewGitRepos(t),
	34	+	git: mocks.NewMergeGit(t),
	35	+	repo: domain.Repo{Name: name(t, "proj"), Visibility: domain.VisibilityPrivate, Settings: domain.DefaultRepoSettings()},
	36	+	}
	37	+	clock := mocks.NewClock(t)
	38	+	clock.EXPECT().Now().Return(now).Maybe()
	39	+	u.mrs = app.NewMergeRequests(app.MergeRequestsConfig{
	40	+	Meta: u.meta, Store: u.store, Repos: u.repos, Git: u.git,
	41	+	Signer: mocks.NewCommitSigner(t), Bus: mocks.NewEventBus(t), Clock: clock,
	42	+	})
	43	+	return u
	44	+	}
	45	+
	46	+	// GO-004: a failure to read Git is an error for the log, not a field violation
	47	+	// that would show its text to the caller.
	48	+	func TestCreateReturnsGitFailuresAsErrors(t *testing.T) {
	49	+	boom := errors.New("fatal: /srv/ermite/repos/proj.git: permission denied")
	50	+	params := app.CreateMergeRequestParams{Title: "t", Source: "feat", Target: "main"}
	51	+
	52	+	t.Run("reading a branch", func(t *testing.T) {
	53	+	u := newMRUnit(t)
	54	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	55	+	u.git.EXPECT().BranchHead(mock.Anything, u.repo.Name, "feat").Return(domain.ObjectID{}, boom)
	56	+	_, err := u.mrs.Create(context.Background(), domain.Root(), u.repo.Name, params)
	57	+	require.ErrorIs(t, err, boom)
	58	+	var fe *domain.InvalidFieldsError
	59	+	assert.False(t, errors.As(err, &fe), "not a field violation: %v", err)
	60	+	assert.NotErrorIs(t, err, domain.ErrInvalid)
	61	+	})
	62	+
	63	+	t.Run("reading the default branch", func(t *testing.T) {
	64	+	u := newMRUnit(t)
	65	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	66	+	u.repos.EXPECT().DefaultBranch(mock.Anything, u.repo.Name).Return("", false, boom)
	67	+	_, err := u.mrs.Create(context.Background(), domain.Root(), u.repo.Name, app.CreateMergeRequestParams{Title: "t", Source: "feat"})
	68	+	require.ErrorIs(t, err, boom)
	69	+	var fe *domain.InvalidFieldsError
	70	+	assert.False(t, errors.As(err, &fe), "not a field violation: %v", err)
	71	+	})
	72	+
	73	+	t.Run("a branch that does not exist stays a violation", func(t *testing.T) {
	74	+	u := newMRUnit(t)
	75	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil)
	76	+	u.git.EXPECT().BranchHead(mock.Anything, u.repo.Name, "feat").Return(domain.ObjectID{}, domain.ErrNotFound)
	77	+	u.git.EXPECT().BranchHead(mock.Anything, u.repo.Name, "main").Return(someID(t), nil)
	78	+	_, err := u.mrs.Create(context.Background(), domain.Root(), u.repo.Name, params)
	79	+	assert.Contains(t, fieldsOf(t, err), "source_branch")
	80	+	})
	81	+	}
	82	+
	83	+	func someID(t *testing.T) domain.ObjectID {
	84	+	t.Helper()
	85	+	return oidFrom(t, oidA)
	86	+	}
	87	+
	88	+	// GO-003: the settings are read again under the merge lock; a failure of that
	89	+	// read is returned, and a repository deleted meanwhile is not found.
	90	+	func TestMergeReturnsTheFailureToReadTheSettings(t *testing.T) {
	91	+	boom := errors.New("kv: closed")
	92	+	for label, tc := range map[string]struct{ err, want error }{
	93	+	"read failure": {boom, boom},
	94	+	"deleted meanwhile": {domain.ErrNotFound, domain.ErrNotFound},
	95	+	} {
	96	+	t.Run(label, func(t *testing.T) {
	97	+	u := newMRUnit(t)
	98	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(u.repo, nil).Once()
	99	+	u.meta.EXPECT().Get(mock.Anything, u.repo.Name).Return(domain.Repo{}, tc.err).Once()
	100	+	u.store.EXPECT().Get(mock.Anything, u.repo.Name, int64(1)).Return(domain.MergeRequest{
	101	+	Repo: u.repo.Name, Number: 1, State: domain.MRStateOpen, Source: "feat", Target: "main",
	102	+	}, nil)
	103	+	_, err := u.mrs.Merge(context.Background(), domain.Root(), u.repo.Name, 1, app.MergeParams{HeadID: someID(t)})
	104	+	require.ErrorIs(t, err, tc.want)
	105	+	if tc.want == boom {
	106	+	assert.NotErrorIs(t, err, domain.ErrNotFound)
	107	+	}
	108	+	})
	109	+	}
	110	+	}