From ff16938096b54efe8812bb8aa683b9d880db13c3 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 17 Oct 2026 12:20:40 +0200 Subject: [PATCH] Leave a held merge unsettled when the controller ends or the hold fails, never take it (issue 921) --- internal/link/receive_fake_test.go | 4 ++++ internal/link/serve.go | 22 ++++++++++++++-------- internal/link/store_window_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/internal/link/receive_fake_test.go b/internal/link/receive_fake_test.go index f7023f32..38f06f01 100644 --- a/internal/link/receive_fake_test.go +++ b/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() } diff --git a/internal/link/serve.go b/internal/link/serve.go index 12ad088b..685df095 100644 --- a/internal/link/serve.go +++ b/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() diff --git a/internal/link/store_window_test.go b/internal/link/store_window_test.go index 09b5ce50..6eb8dea7 100644 --- a/internal/link/store_window_test.go +++ b/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) + } +}