6793112 fix(mirror): skip non-UTF-8 ref names, keep SyncAll errors to the failure class and document the Wait barrier · ermite

M internal/adapters/rpc/mirror.go +3 -3


@@ -146,12 +146,12 @@ func (s mirrorService) SyncMirror(ctx context.Context, req connect.Request[pb.
146	146		return connect.NewResponse(&pb.SyncMirrorResponse{Results: out}), nil
147	147		}
148	148
149		-	// syncErrorText is the human reason a repository failed in SyncAll. The caller is an owner;
150		-	// a mirror error is already redacted by the transport and carries no URL.
	149	+	// syncErrorText is the short reason a repository failed in SyncAll: the failure class only.
	150	+	// The remote's output stays in GetMirrorStatus, so the response stays small and URL-free.
151	151		func syncErrorText(err error) string {
152	152		var me *domain.MirrorError
153	153		if errors.As(err, &me) {
154		-	return "mirror " + string(me.Class) + ": " + me.Detail
	154	+	return "mirror " + string(me.Class) + ": see ermite mirror status"
155	155		}
156	156		switch {
157	157		case errors.Is(err, domain.ErrResourceExhausted):


@@ -146,12 +146,12 @@ func (s mirrorService) SyncMirror(ctx context.Context, req connect.Request[pb.
146		return connect.NewResponse(&pb.SyncMirrorResponse{Results: out}), nil	146		return connect.NewResponse(&pb.SyncMirrorResponse{Results: out}), nil
147		}	147		}
148			148
149	-	// syncErrorText is the human reason a repository failed in SyncAll. The caller is an owner;	149	+	// syncErrorText is the short reason a repository failed in SyncAll: the failure class only.
150	-	// a mirror error is already redacted by the transport and carries no URL.	150	+	// The remote's output stays in GetMirrorStatus, so the response stays small and URL-free.
151		func syncErrorText(err error) string {	151		func syncErrorText(err error) string {
152		var me *domain.MirrorError	152		var me *domain.MirrorError
153		if errors.As(err, &me) {	153		if errors.As(err, &me) {
154	-	return "mirror " + string(me.Class) + ": " + me.Detail	154	+	return "mirror " + string(me.Class) + ": see ermite mirror status"
155		}	155		}
156		switch {	156		switch {
157		case errors.Is(err, domain.ErrResourceExhausted):	157		case errors.Is(err, domain.ErrResourceExhausted):

M internal/adapters/rpc/mirror_test.go +1 -1


@@ -85,7 +85,7 @@ func TestMirrorSyncAllReportsPerRepositoryErrors(t *testing.T) {
85	85		require.NoError(t, err)
86	86		require.Len(t, res.Msg.Results, 2)
87	87		assert.Empty(t, res.Msg.Results[0].Error)
88		-	assert.Equal(t, "mirror diverged: rejected", res.Msg.Results[1].Error)
	88	+	assert.Equal(t, "mirror diverged: see ermite mirror status", res.Msg.Results[1].Error)
89	89		assert.Equal(t, pb.MirrorState_MIRROR_STATE_PAUSED, res.Msg.Results[1].Status.State)
90	90		}
91	91


@@ -85,7 +85,7 @@ func TestMirrorSyncAllReportsPerRepositoryErrors(t *testing.T) {
85		require.NoError(t, err)	85		require.NoError(t, err)
86		require.Len(t, res.Msg.Results, 2)	86		require.Len(t, res.Msg.Results, 2)
87		assert.Empty(t, res.Msg.Results[0].Error)	87		assert.Empty(t, res.Msg.Results[0].Error)
88	-	assert.Equal(t, "mirror diverged: rejected", res.Msg.Results[1].Error)	88	+	assert.Equal(t, "mirror diverged: see ermite mirror status", res.Msg.Results[1].Error)
89		assert.Equal(t, pb.MirrorState_MIRROR_STATE_PAUSED, res.Msg.Results[1].Status.State)	89		assert.Equal(t, pb.MirrorState_MIRROR_STATE_PAUSED, res.Msg.Results[1].Status.State)
90		}	90		}
91			91

M internal/domain/mirror_refs.go +3 -1


@@ -6,6 +6,7 @@ import (
6	6		"sort"
7	7		"strings"
8	8		"time"
	9	+	"unicode/utf8"
9	10		)
10	11
11	12		type MirrorRef struct {

@@ -23,11 +24,12 @@ type MirrorSnapshot struct {
23	24		// MirrorRefspecs selects refs/heads/<default> (or every refs/heads/* with allBranches) and
24	25		// refs/tags/*, from the refs of RepoReader.ListRefs (HEAD skipped, unborn default skipped).
25	26		// Names are plain refs; the caller builds "<ref>:<ref>" (never "+", never a delete).
	27	+	// Refs whose name is not valid UTF-8 are skipped: they cannot be stored in the snapshot.
26	28		// Returned sorted by ref name.
27	29		func MirrorRefspecs(refs []Ref, defaultBranch string, allBranches bool) []MirrorRef {
28	30		var out []MirrorRef
29	31		for _, r := range refs {
30		-	if r.ID.IsZero() \|\| r.ID.String() == ZeroOID {
	32	+	if r.ID.IsZero() \|\| r.ID.String() == ZeroOID \|\| !utf8.ValidString(r.Name) {
31	33		continue
32	34		}
33	35		if strings.HasPrefix(r.Name, "refs/tags/") \|\| strings.HasPrefix(r.Name, "refs/heads/") && (allBranches \|\| r.Name == "refs/heads/"+defaultBranch) {

 	"sort"
 	"strings"
 	"time"
+	"unicode/utf8"
 )
 type MirrorRef struct {
 // MirrorRefspecs selects refs/heads/<default> (or every refs/heads/* with allBranches) and
 // refs/tags/*, from the refs of RepoReader.ListRefs (HEAD skipped, unborn default skipped).
 // Names are plain refs; the caller builds "<ref>:<ref>" (never "+", never a delete).
+// Refs whose name is not valid UTF-8 are skipped: they cannot be stored in the snapshot.
 // Returned sorted by ref name.
 func MirrorRefspecs(refs []Ref, defaultBranch string, allBranches bool) []MirrorRef {
 	var out []MirrorRef
 	for _, r := range refs {
-		if r.ID.IsZero() || r.ID.String() == ZeroOID {
+		if r.ID.IsZero() || r.ID.String() == ZeroOID || !utf8.ValidString(r.Name) {
 			continue
 		}
 		if strings.HasPrefix(r.Name, "refs/tags/") || strings.HasPrefix(r.Name, "refs/heads/") && (allBranches || r.Name == "refs/heads/"+defaultBranch) {

M internal/domain/mirror_test.go +1 -1


@@ -240,7 +240,7 @@ func TestMirrorRecoveryFromAnyState(t *testing.T) {
240	240		}
241	241		func TestMirrorRefs(t *testing.T) {
242	242		id, _ := ParseObjectID(strings.Repeat("a", 40))
243		-	refs := []Ref{{Name: "HEAD", ID: id}, {Name: "refs/heads/main", ID: id}, {Name: "refs/heads/other", ID: id}, {Name: "refs/tags/v1", ID: id}, {Name: "refs/pull/1", ID: id}, {Name: "refs/heads/unborn"}}
	243	+	refs := []Ref{{Name: "HEAD", ID: id}, {Name: "refs/heads/main", ID: id}, {Name: "refs/heads/other", ID: id}, {Name: "refs/tags/v1", ID: id}, {Name: "refs/pull/1", ID: id}, {Name: "refs/heads/unborn"}, {Name: "refs/tags/\xff", ID: id}, {Name: "refs/heads/\xff", ID: id}}
244	244		for _, all := range []bool{false, true} {
245	245		got := MirrorRefspecs(refs, "main", all)
246	246		n := 2


@@ -240,7 +240,7 @@ func TestMirrorRecoveryFromAnyState(t *testing.T) {
240		}	240		}
241		func TestMirrorRefs(t *testing.T) {	241		func TestMirrorRefs(t *testing.T) {
242		id, _ := ParseObjectID(strings.Repeat("a", 40))	242		id, _ := ParseObjectID(strings.Repeat("a", 40))
243	-	refs := []Ref{{Name: "HEAD", ID: id}, {Name: "refs/heads/main", ID: id}, {Name: "refs/heads/other", ID: id}, {Name: "refs/tags/v1", ID: id}, {Name: "refs/pull/1", ID: id}, {Name: "refs/heads/unborn"}}	243	+	refs := []Ref{{Name: "HEAD", ID: id}, {Name: "refs/heads/main", ID: id}, {Name: "refs/heads/other", ID: id}, {Name: "refs/tags/v1", ID: id}, {Name: "refs/pull/1", ID: id}, {Name: "refs/heads/unborn"}, {Name: "refs/tags/\xff", ID: id}, {Name: "refs/heads/\xff", ID: id}}
244		for _, all := range []bool{false, true} {	244		for _, all := range []bool{false, true} {
245		got := MirrorRefspecs(refs, "main", all)	245		got := MirrorRefspecs(refs, "main", all)
246		n := 2	246		n := 2

M internal/usecase/mirrors_sync.go +4 -3


@@ -97,8 +97,8 @@ func (m *Mirrors) SyncAll(ctx context.Context, p domain.Principal, force bool) (
97	97		return out, nil
98	98		}
99	99
100		-	// Wait blocks until no sync is queued or running, or ctx is done. It is for tests and shutdown
101		-	// paths that need a deterministic barrier.
	100	+	// Wait blocks until no sync is queued or running, or ctx is done. It is a test barrier: call it
	101	+	// only while nothing enqueues new syncs and before Close (queued jobs are not drained by Close).
102	102		func (m *Mirrors) Wait(ctx context.Context) {
103	103		done := make(chan struct{})
104	104		go func() { m.flightWG.Wait(); close(done) }()

@@ -254,7 +254,8 @@ func (m *Mirrors) attempt(ctx context.Context, repo domain.RepoName, req mirrorR
254	254		}
255	255
256	256		// record stores the outcome, unless the effective target changed during the push: the result
257		-	// then describes a target that no longer applies and is discarded.
	257	+	// then describes a target that no longer applies and is discarded. The comparison includes the
	258	+	// cron on purpose, like resetIfChanged: a schedule change during a push costs one extra push.
258	259		func (m Mirrors) record(ctx context.Context, repo domain.RepoName, pl mirrorPlan, selected []domain.MirrorRef, noOp bool, failure domain.MirrorError, runErr error) (ports.MirrorSyncResult, error) {
259	260		m.configMu.RLock()
260	261		defer m.configMu.RUnlock()

 	return out, nil
 }
-// Wait blocks until no sync is queued or running, or ctx is done. It is for tests and shutdown
+// Wait blocks until no sync is queued or running, or ctx is done. It is a test barrier: call it
-// paths that need a deterministic barrier.
+// only while nothing enqueues new syncs and before Close (queued jobs are not drained by Close).
 func (m *Mirrors) Wait(ctx context.Context) {
 	done := make(chan struct{})
 	go func() { m.flightWG.Wait(); close(done) }()
 }
 // record stores the outcome, unless the effective target changed during the push: the result
-// then describes a target that no longer applies and is discarded.
+// then describes a target that no longer applies and is discarded. The comparison includes the
+// cron on purpose, like resetIfChanged: a schedule change during a push costs one extra push.
 func (m *Mirrors) record(ctx context.Context, repo domain.RepoName, pl mirrorPlan, selected []domain.MirrorRef, noOp bool, failure *domain.MirrorError, runErr error) (ports.MirrorSyncResult, error) {
 	m.configMu.RLock()
 	defer m.configMu.RUnlock()