fix(app): publish repo updates when only the HEAD move fails

commit: 13c62e2a795eb2c5adf050232b53640c8691fe3b
author: brice · 12d ago · 2026-10-10 11:41 UTC
parent: 19ceb08
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +34 -9

M internal/app/repos_write.go +17 -6


@@ -64,7 +64,8 @@ type UpdateParams struct {
64	64		// else domain.ErrInvalid. A default branch that does not exist gives
65	65		// domain.ErrPreconditionFailed; nothing is written before every check passed.
66	66		// The metadata is written first, then HEAD, and RepoUpdated is published after
67		-	// both (see the comment on the writes for the partial state).
	67	+	// both. When only the HEAD move fails, RepoUpdated is still published for the
	68	+	// stored metadata and the HEAD error is returned.
68	69		func (r *Repos) Update(ctx context.Context, p domain.Principal, name domain.RepoName, params UpdateParams) (domain.Repo, error) {
69	70		repo, err := lookupRepo(ctx, r.meta, name)
70	71		if err != nil {

@@ -101,18 +102,25 @@ func (r *Repos) Update(ctx context.Context, p domain.Principal, name domain.Repo
101	102		}
102	103
103	104		// Metadata first, then HEAD. The two writes are not atomic: when moving
104		-	// HEAD fails after the metadata was stored, the call fails with the
105		-	// description and visibility already changed, and no RepoUpdated is
106		-	// published for that partial state. The branch was checked above, so the
107		-	// remaining causes are a vanished repository or an I/O failure.
	105	+	// HEAD fails after the metadata was stored, the stored change is still
	106	+	// published as RepoUpdated (a repository made private must be withdrawn
	107	+	// from the watchers) and the call then fails with the HEAD error. The
	108	+	// branch was checked above, so the remaining causes are a vanished
	109	+	// repository or an I/O failure.
	110	+	metaDone := false
108	111		if params.Description != nil \|\| params.Visibility != nil {
109	112		if repo, err = r.updateMeta(ctx, name, params); err != nil {
110	113		return domain.Repo{}, err
111	114		}
	115	+	metaDone = true
112	116		}
	117	+	var headErr error
113	118		if params.DefaultBranch != nil {
114	119		if err := r.git.SetDefaultBranch(ctx, name, *params.DefaultBranch); err != nil {
115		-	return domain.Repo{}, fmt.Errorf("set default branch of %q: %w", name, err)
	120	+	headErr = fmt.Errorf("set default branch of %q: %w", name, err)
	121	+	if !metaDone {
	122	+	return domain.Repo{}, headErr
	123	+	}
116	124		}
117	125		}
118	126

@@ -128,6 +136,9 @@ func (r *Repos) Update(ctx context.Context, p domain.Principal, name domain.Repo
128	136		Repo: name, At: r.clock.Now().UTC(),
129	137		Visibility: repo.Visibility, Previous: before.Visibility, CreatedAt: repo.CreatedAt,
130	138		})
	139	+	if headErr != nil {
	140	+	return domain.Repo{}, headErr
	141	+	}
131	142		return repo, nil
132	143		}
133	144

 // else domain.ErrInvalid. A default branch that does not exist gives
 // domain.ErrPreconditionFailed; nothing is written before every check passed.
 // The metadata is written first, then HEAD, and RepoUpdated is published after
-// both (see the comment on the writes for the partial state).
+// both. When only the HEAD move fails, RepoUpdated is still published for the
+// stored metadata and the HEAD error is returned.
 func (r *Repos) Update(ctx context.Context, p domain.Principal, name domain.RepoName, params UpdateParams) (domain.Repo, error) {
 	repo, err := lookupRepo(ctx, r.meta, name)
 	if err != nil {
 	}
 	// Metadata first, then HEAD. The two writes are not atomic: when moving
-	// HEAD fails after the metadata was stored, the call fails with the
+	// HEAD fails after the metadata was stored, the stored change is still
-	// description and visibility already changed, and no RepoUpdated is
+	// published as RepoUpdated (a repository made private must be withdrawn
-	// published for that partial state. The branch was checked above, so the
+	// from the watchers) and the call then fails with the HEAD error. The
-	// remaining causes are a vanished repository or an I/O failure.
+	// branch was checked above, so the remaining causes are a vanished
+	// repository or an I/O failure.
+	metaDone := false
 	if params.Description != nil || params.Visibility != nil {
 		if repo, err = r.updateMeta(ctx, name, params); err != nil {
 			return domain.Repo{}, err
 		}
+		metaDone = true
 	}
+	var headErr error
 	if params.DefaultBranch != nil {
 		if err := r.git.SetDefaultBranch(ctx, name, *params.DefaultBranch); err != nil {
-			return domain.Repo{}, fmt.Errorf("set default branch of %q: %w", name, err)
+			headErr = fmt.Errorf("set default branch of %q: %w", name, err)
+			if !metaDone {
+				return domain.Repo{}, headErr
+			}
 		}
 	}
 		Repo: name, At: r.clock.Now().UTC(),
 		Visibility: repo.Visibility, Previous: before.Visibility, CreatedAt: repo.CreatedAt,
 	})
+	if headErr != nil {
+		return domain.Repo{}, headErr
+	}
 	return repo, nil
 }

M internal/app/repos_write_test.go +17 -3


@@ -80,14 +80,28 @@ func TestUpdate(t *testing.T) {
80	80		assert.Equal(t, []string{"meta", "head", "event"}, order)
81	81		})
82	82
83		-	t.Run("a HEAD that cannot move fails the call and publishes nothing", func(t *testing.T) {
	83	+	t.Run("a HEAD that cannot move after the metadata was stored still publishes, then fails", func(t *testing.T) {
84	84		f := newFixture(t)
85	85		boom := errors.New("boom")
86		-	f.meta.EXPECT().Get(ctx, n).Return(priv, nil)
	86	+	pub := domain.Repo{Name: n, Visibility: domain.VisibilityPublic, Description: "old"}
	87	+	f.meta.EXPECT().Get(ctx, n).Return(pub, nil)
87	88		f.git.EXPECT().HasBranch(ctx, n, "dev").Return(true, nil)
88	89		f.meta.EXPECT().Update(ctx, mock.Anything).Return(nil)
89	90		f.git.EXPECT().SetDefaultBranch(ctx, n, "dev").Return(boom)
90		-	_, err := f.repos.Update(ctx, admin, n, app.UpdateParams{Description: ptr("x"), DefaultBranch: ptr("dev")})
	91	+	f.bus.EXPECT().Publish(ctx, domain.RepoUpdated{
	92	+	Repo: n, At: now, Visibility: domain.VisibilityPrivate, Previous: domain.VisibilityPublic,
	93	+	}).Return().Once()
	94	+	_, err := f.repos.Update(ctx, admin, n, app.UpdateParams{Visibility: ptr(domain.VisibilityPrivate), DefaultBranch: ptr("dev")})
	95	+	assert.ErrorIs(t, err, boom)
	96	+	})
	97	+
	98	+	t.Run("a HEAD that cannot move with no metadata change publishes nothing", func(t *testing.T) {
	99	+	f := newFixture(t)
	100	+	boom := errors.New("boom")
	101	+	f.meta.EXPECT().Get(ctx, n).Return(priv, nil)
	102	+	f.git.EXPECT().HasBranch(ctx, n, "dev").Return(true, nil)
	103	+	f.git.EXPECT().SetDefaultBranch(ctx, n, "dev").Return(boom)
	104	+	_, err := f.repos.Update(ctx, admin, n, app.UpdateParams{DefaultBranch: ptr("dev")})
91	105		assert.ErrorIs(t, err, boom)
92	106		})
93	107

 		assert.Equal(t, []string{"meta", "head", "event"}, order)
 	})
-	t.Run("a HEAD that cannot move fails the call and publishes nothing", func(t *testing.T) {
+	t.Run("a HEAD that cannot move after the metadata was stored still publishes, then fails", func(t *testing.T) {
 		f := newFixture(t)
 		boom := errors.New("boom")
-		f.meta.EXPECT().Get(ctx, n).Return(priv, nil)
+		pub := domain.Repo{Name: n, Visibility: domain.VisibilityPublic, Description: "old"}
+		f.meta.EXPECT().Get(ctx, n).Return(pub, nil)
 		f.git.EXPECT().HasBranch(ctx, n, "dev").Return(true, nil)
 		f.meta.EXPECT().Update(ctx, mock.Anything).Return(nil)
 		f.git.EXPECT().SetDefaultBranch(ctx, n, "dev").Return(boom)
-		_, err := f.repos.Update(ctx, admin, n, app.UpdateParams{Description: ptr("x"), DefaultBranch: ptr("dev")})
+		f.bus.EXPECT().Publish(ctx, domain.RepoUpdated{
+			Repo: n, At: now, Visibility: domain.VisibilityPrivate, Previous: domain.VisibilityPublic,
+		}).Return().Once()
+		_, err := f.repos.Update(ctx, admin, n, app.UpdateParams{Visibility: ptr(domain.VisibilityPrivate), DefaultBranch: ptr("dev")})
+		assert.ErrorIs(t, err, boom)
+	})
+	t.Run("a HEAD that cannot move with no metadata change publishes nothing", func(t *testing.T) {
+		f := newFixture(t)
+		boom := errors.New("boom")
+		f.meta.EXPECT().Get(ctx, n).Return(priv, nil)
+		f.git.EXPECT().HasBranch(ctx, n, "dev").Return(true, nil)
+		f.git.EXPECT().SetDefaultBranch(ctx, n, "dev").Return(boom)
+		_, err := f.repos.Update(ctx, admin, n, app.UpdateParams{DefaultBranch: ptr("dev")})
 		assert.ErrorIs(t, err, boom)
 	})