Leave a held merge unsettled when the controller ends or the hold fails, never take it (issue 921)

internal/link/receive_fake_test.go

@@ -68,6 +68,7 @@ type fakeControl struct {
 	on          *fakeInbound
 	redelivered bool
 	since       time.Time
+	holdFails   error
 }
 
 func (m *fakeControl) Kind() string                         { return m.kind }
@@ -88,6 +89,9 @@ func (m *fakeControl) HeldFor() time.Duration {
 }
 
 func (m *fakeControl) Hold(time.Duration) error {
+	if m.holdFails != nil {
+		return m.holdFails
+	}
 	if m.since.IsZero() {
 		m.since = time.Now()
 	}

internal/link/serve.go

@@ -657,16 +657,22 @@ func (s *Server) sourceMoved(ctx context.Context, m Control) {
 		return
 	}
 	err := s.upgrader.SourceMoved(ctx, moved)
-	if err != nil && errors.Is(err, ErrTryAgain) && ctx.Err() == nil {
-		if held := m.Hold(mergeHeldFor); held == nil {
-			s.log.Printf("%s/%s moved to %.8s and is held: %v; handed over again in %s", moved.Owner, moved.Repo,
-				moved.Commit, err, mergeHeldFor)
+	if err != nil && errors.Is(err, ErrTryAgain) {
+		// Left unsettled when the controller is ending (the bus hands it to whatever consumes next, as decide
+		// does) and when the bus will not hold it: taking it would lose the merge.
+		if ctx.Err() != nil {
 			return
-		} else {
-			s.log.Printf("LOST %s/%s moved to %.8s: it could not be held (%v): %v", moved.Owner, moved.Repo,
-				moved.Commit, held, err)
 		}
-	} else if err != nil {
+		if held := m.Hold(mergeHeldFor); held != nil {
+			s.log.Printf("%s/%s moved to %.8s and could not be held (%v): %v; left for the bus to hand over again",
+				moved.Owner, moved.Repo, moved.Commit, held, err)
+			return
+		}
+		s.log.Printf("%s/%s moved to %.8s and is held: %v; handed over again in %s", moved.Owner, moved.Repo,
+			moved.Commit, err, mergeHeldFor)
+		return
+	}
+	if err != nil {
 		s.log.Printf("%s/%s moved to %.8s and the mesh could not act on it: %v", moved.Owner, moved.Repo, moved.Commit, err)
 	}
 	_ = m.Took()

internal/link/store_window_test.go

@@ -136,3 +136,30 @@ func TestAMergeThatCouldNotBeRecordedNowIsHeldNotDropped(t *testing.T) {
 		}
 	}
 }
+
+// A merge being held when the controller is replaced (its context ended) or when the bus will not hold it is left
+// unsettled, never taken: the bus hands it to whatever consumes next (review of issue 921).
+func TestAMergeIsLeftUnsettledWhenTheControllerEndsOrTheHoldFails(t *testing.T) {
+	merged := SourceMoved{Owner: "novox", Repo: "one", Base: "main", Commit: "abcdef0123"}
+	try := errors.Join(ErrTryAgain, errors.New("another controller is working the plans"))
+
+	s, in := serving()
+	s.upgrader = upgradesWith{err: try}
+	to := &settled{}
+	ended, end := context.WithCancel(context.Background())
+	end()
+	s.act(ended, in.sends(t, to, KindSourceMoved, merged))
+	if !to.unsettled() {
+		t.Errorf("a merge was settled as %+v when the controller's context ended", *to)
+	}
+
+	s, in = serving()
+	s.upgrader = upgradesWith{err: try}
+	to = &settled{}
+	m := in.sends(t, to, KindSourceMoved, merged).(*fakeControl)
+	m.holdFails = errors.New("the bus will not hold it")
+	s.act(context.Background(), m)
+	if !to.unsettled() {
+		t.Errorf("a merge was settled as %+v when it could not be held", *to)
+	}
+}