detached 6609752

fix(mr): resolve the auto-merge enabler under the merge lock

commit
66097520201230c31ddd90ed344ebedde90c8b92
author
brice · · 2026-10-20 16:32 UTC
parent
8539d6f
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
3 files · +48 -12
Split
UnifiedSplit
M internal/usecase/automerge.go +1 -8
@@ -228,13 +228,6 @@ func (s *AutoMerge) evaluateMR(ctx context.Context, repo domain.Repo, mr domain.
228228 if head != intent.HeadID {
229229 return cancel("the branch moved to " + head.String()[:7])
230230 }
231- p, valid, err := principalOfPusher(ctx, s.cfg.Tokens, intent.EnabledBy, s.cfg.Clock.Now())
232- if err != nil {
233- return err
234- }
235- if !valid || !p.Allows(domain.ScopeMRMerge, mr.Repo) || !p.AllowsRef("refs/heads/"+mr.Target) {
236- return cancel("the principal that enabled auto-merge can no longer merge " + mr.Target)
237- }
238231 if s.cfg.Activity != nil && !s.cfg.Activity.Settled(ctx, mr.Repo, mr.Number, mr.Source, head) {
239232 return nil
240233 }
@@ -252,7 +245,7 @@ func (s *AutoMerge) evaluateMR(ctx context.Context, repo domain.Repo, mr domain.
252245 if !passed {
253246 return nil
254247 }
255- _, err = s.cfg.MRs.mergeAs(ctx, p, mr.Repo, mr.Number, ports.MergeParams{Mode: intent.Mode, HeadID: intent.HeadID}, mergeOpts{auto: true, intent: intent})
248+ _, err = s.cfg.MRs.mergeAs(ctx, domain.Principal{}, mr.Repo, mr.Number, ports.MergeParams{Mode: intent.Mode, HeadID: intent.HeadID}, mergeOpts{auto: true, intent: intent, enabler: intent.EnabledBy, tokens: s.cfg.Tokens, activity: s.cfg.Activity})
256249 switch {
257250 case err == nil, errors.Is(err, errAutoMergeWait):
258251 return nil
@@ -228,13 +228,6 @@ func (s *AutoMerge) evaluateMR(ctx context.Context, repo domain.Repo, mr domain.
228 if head != intent.HeadID {228 if head != intent.HeadID {
229 return cancel("the branch moved to " + head.String()[:7])229 return cancel("the branch moved to " + head.String()[:7])
230 }230 }
231- p, valid, err := principalOfPusher(ctx, s.cfg.Tokens, intent.EnabledBy, s.cfg.Clock.Now())
232- if err != nil {
233- return err
234- }
235- if !valid || !p.Allows(domain.ScopeMRMerge, mr.Repo) || !p.AllowsRef("refs/heads/"+mr.Target) {
236- return cancel("the principal that enabled auto-merge can no longer merge " + mr.Target)
237- }
238 if s.cfg.Activity != nil && !s.cfg.Activity.Settled(ctx, mr.Repo, mr.Number, mr.Source, head) {231 if s.cfg.Activity != nil && !s.cfg.Activity.Settled(ctx, mr.Repo, mr.Number, mr.Source, head) {
239 return nil232 return nil
240 }233 }
@@ -252,7 +245,7 @@ func (s *AutoMerge) evaluateMR(ctx context.Context, repo domain.Repo, mr domain.
252 if !passed {245 if !passed {
253 return nil246 return nil
254 }247 }
255- _, err = s.cfg.MRs.mergeAs(ctx, p, mr.Repo, mr.Number, ports.MergeParams{Mode: intent.Mode, HeadID: intent.HeadID}, mergeOpts{auto: true, intent: intent})248+ _, err = s.cfg.MRs.mergeAs(ctx, domain.Principal{}, mr.Repo, mr.Number, ports.MergeParams{Mode: intent.Mode, HeadID: intent.HeadID}, mergeOpts{auto: true, intent: intent, enabler: intent.EnabledBy, tokens: s.cfg.Tokens, activity: s.cfg.Activity})
256 switch {249 switch {
257 case err == nil, errors.Is(err, errAutoMergeWait):250 case err == nil, errors.Is(err, errAutoMergeWait):
258 return nil251 return nil
M internal/usecase/automerge_test.go +28 -0
@@ -208,6 +208,7 @@ func TestAutoMergeCancelsWhenEnablerTokenRevokedOrExpired(t *testing.T) {
208208 enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "gone"}
209209 require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
210210 tokens := mocks.NewTokenStore(t)
211+ checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil)
211212 if expired {
212213 tokens.EXPECT().Get(mock.Anything, "gone").Return(domain.Token{ExpiresAt: now.Add(-time.Hour)}, nil)
213214 } else {
@@ -396,3 +397,30 @@ func TestMergeGateSettingsUpdateWaits(t *testing.T) {
396397 require.NoError(t, err)
397398 }
398399 }
400+
401+func TestAutoMergeResolvesEnablerAfterMergeGate(t *testing.T) {
402+ r, mr, checks := autoRig(t)
403+ enableAuto(t, r, mr)
404+ enabled := storedAuto(t, r, mr)
405+ enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "revoked"}
406+ require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
407+ tokens := mocks.NewTokenStore(t)
408+ tokens.EXPECT().Get(mock.Anything, "revoked").Return(domain.Token{}, domain.ErrNotFound).Once()
409+ reached := make(chan struct{})
410+ checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Run(func(context.Context, domain.RepoName, string) { close(reached) }).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil).Once()
411+ a := autoEvaluator(r, checks, nil, tokens)
412+ defer a.Close(r.ctx)
413+ done := make(chan bool, 1)
414+ require.NoError(t, r.mrs.WithMergeLock(r.ctx, r.repo, func() error {
415+ go func() { done <- a.Evaluate(r.repo) }()
416+ <-reached
417+ // Evaluation has passed its fast-path checks, but cannot resolve the token
418+ // until this lock is released. It now sees the revoked token, not old rights.
419+ tokens.AssertNotCalled(t, "Get", mock.Anything, "revoked")
420+ return nil
421+ }))
422+ require.True(t, <-done)
423+ got := storedAuto(t, r, mr)
424+ require.Nil(t, got.AutoMerge)
425+ require.True(t, got.IsOpen())
426+}
@@ -208,6 +208,7 @@ func TestAutoMergeCancelsWhenEnablerTokenRevokedOrExpired(t *testing.T) {
208 enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "gone"}208 enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "gone"}
209 require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))209 require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
210 tokens := mocks.NewTokenStore(t)210 tokens := mocks.NewTokenStore(t)
211+ checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil)
211 if expired {212 if expired {
212 tokens.EXPECT().Get(mock.Anything, "gone").Return(domain.Token{ExpiresAt: now.Add(-time.Hour)}, nil)213 tokens.EXPECT().Get(mock.Anything, "gone").Return(domain.Token{ExpiresAt: now.Add(-time.Hour)}, nil)
213 } else {214 } else {
@@ -396,3 +397,30 @@ func TestMergeGateSettingsUpdateWaits(t *testing.T) {
396 require.NoError(t, err)397 require.NoError(t, err)
397 }398 }
398 }399 }
400+
401+func TestAutoMergeResolvesEnablerAfterMergeGate(t *testing.T) {
402+ r, mr, checks := autoRig(t)
403+ enableAuto(t, r, mr)
404+ enabled := storedAuto(t, r, mr)
405+ enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "revoked"}
406+ require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
407+ tokens := mocks.NewTokenStore(t)
408+ tokens.EXPECT().Get(mock.Anything, "revoked").Return(domain.Token{}, domain.ErrNotFound).Once()
409+ reached := make(chan struct{})
410+ checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Run(func(context.Context, domain.RepoName, string) { close(reached) }).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil).Once()
411+ a := autoEvaluator(r, checks, nil, tokens)
412+ defer a.Close(r.ctx)
413+ done := make(chan bool, 1)
414+ require.NoError(t, r.mrs.WithMergeLock(r.ctx, r.repo, func() error {
415+ go func() { done <- a.Evaluate(r.repo) }()
416+ <-reached
417+ // Evaluation has passed its fast-path checks, but cannot resolve the token
418+ // until this lock is released. It now sees the revoked token, not old rights.
419+ tokens.AssertNotCalled(t, "Get", mock.Anything, "revoked")
420+ return nil
421+ }))
422+ require.True(t, <-done)
423+ got := storedAuto(t, r, mr)
424+ require.Nil(t, got.AutoMerge)
425+ require.True(t, got.IsOpen())
426+}
M internal/usecase/mergerequests.go +19 -4
@@ -1041,15 +1041,20 @@ func (m *MergeRequests) checkMode(ctx context.Context, mr domain.MergeRequest, m
10411041 }
10421042
10431043 type mergeOpts struct {
1044- auto bool
1045- intent *domain.AutoMerge
1044+ auto bool
1045+ intent *domain.AutoMerge
1046+ enabler domain.CIPusher
1047+ tokens ports.TokenStore
1048+ activity ports.CIActivity
10461049 }
10471050
10481051 var errAutoMergeWait = errors.New("auto-merge is waiting for checks")
10491052
10501053 func (m *MergeRequests) mergeAs(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params ports.MergeParams, opts mergeOpts) (domain.MergeRequest, error) {
1051- if _, err := m.writable(ctx, p, name, domain.ScopeMRMerge); err != nil {
1052- return domain.MergeRequest{}, err
1054+ if !opts.auto {
1055+ if _, err := m.writable(ctx, p, name, domain.ScopeMRMerge); err != nil {
1056+ return domain.MergeRequest{}, err
1057+ }
10531058 }
10541059 mode := params.Mode
10551060 if mode == "" {
@@ -1081,6 +1086,16 @@ func (m *MergeRequests) mergeAs(ctx context.Context, p domain.Principal, name do
10811086 if opts.auto && (mr.AutoMerge == nil || opts.intent == nil || *mr.AutoMerge != *opts.intent) {
10821087 return domain.MergeRequest{}, errAutoMergeWait
10831088 }
1089+ if opts.auto {
1090+ fresh, valid, err := principalOfPusher(ctx, opts.tokens, opts.enabler, m.clock.Now())
1091+ if err != nil {
1092+ return domain.MergeRequest{}, err
1093+ }
1094+ if !valid || !fresh.Allows(domain.ScopeMRMerge, name) || !fresh.AllowsRef("refs/heads/"+mr.Target) {
1095+ return domain.MergeRequest{}, fmt.Errorf("%w: the principal that enabled auto-merge can no longer merge %s", domain.ErrForbidden, mr.Target)
1096+ }
1097+ p = fresh
1098+ }
10841099 // A merge moves the target and may delete the source: a token limited to
10851100 // branch prefixes may do so only within them.
10861101 if !p.AllowsRef("refs/heads/" + mr.Target) {
@@ -1041,15 +1041,20 @@ func (m *MergeRequests) checkMode(ctx context.Context, mr domain.MergeRequest, m
1041 }1041 }
1042 1042
1043 type mergeOpts struct {1043 type mergeOpts struct {
1044- auto bool1044+ auto bool
1045- intent *domain.AutoMerge1045+ intent *domain.AutoMerge
1046+ enabler domain.CIPusher
1047+ tokens ports.TokenStore
1048+ activity ports.CIActivity
1046 }1049 }
1047 1050
1048 var errAutoMergeWait = errors.New("auto-merge is waiting for checks")1051 var errAutoMergeWait = errors.New("auto-merge is waiting for checks")
1049 1052
1050 func (m *MergeRequests) mergeAs(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params ports.MergeParams, opts mergeOpts) (domain.MergeRequest, error) {1053 func (m *MergeRequests) mergeAs(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params ports.MergeParams, opts mergeOpts) (domain.MergeRequest, error) {
1051- if _, err := m.writable(ctx, p, name, domain.ScopeMRMerge); err != nil {1054+ if !opts.auto {
1052- return domain.MergeRequest{}, err1055+ if _, err := m.writable(ctx, p, name, domain.ScopeMRMerge); err != nil {
1056+ return domain.MergeRequest{}, err
1057+ }
1053 }1058 }
1054 mode := params.Mode1059 mode := params.Mode
1055 if mode == "" {1060 if mode == "" {
@@ -1081,6 +1086,16 @@ func (m *MergeRequests) mergeAs(ctx context.Context, p domain.Principal, name do
1081 if opts.auto && (mr.AutoMerge == nil || opts.intent == nil || *mr.AutoMerge != *opts.intent) {1086 if opts.auto && (mr.AutoMerge == nil || opts.intent == nil || *mr.AutoMerge != *opts.intent) {
1082 return domain.MergeRequest{}, errAutoMergeWait1087 return domain.MergeRequest{}, errAutoMergeWait
1083 }1088 }
1089+ if opts.auto {
1090+ fresh, valid, err := principalOfPusher(ctx, opts.tokens, opts.enabler, m.clock.Now())
1091+ if err != nil {
1092+ return domain.MergeRequest{}, err
1093+ }
1094+ if !valid || !fresh.Allows(domain.ScopeMRMerge, name) || !fresh.AllowsRef("refs/heads/"+mr.Target) {
1095+ return domain.MergeRequest{}, fmt.Errorf("%w: the principal that enabled auto-merge can no longer merge %s", domain.ErrForbidden, mr.Target)
1096+ }
1097+ p = fresh
1098+ }
1084 // A merge moves the target and may delete the source: a token limited to1099 // A merge moves the target and may delete the source: a token limited to
1085 // branch prefixes may do so only within them.1100 // branch prefixes may do so only within them.
1086 if !p.AllowsRef("refs/heads/" + mr.Target) {1101 if !p.AllowsRef("refs/heads/" + mr.Target) {

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 2

Tags 42

↑↓ moveenter openesc close