fix: clear stale workspace lock tags and improve lock error messages (CLOUD-933)

pkg/tfc_trigger/tfc_trigger.go

@@ -33,9 +33,9 @@ const (
 	InvalidAction
 )
 
-const tfPrefix = "tfbuddylock"
+const TFLockTagPrefix = "tfbuddylock"
 
-var tagRegex = regexp.MustCompile(fmt.Sprintf("%s\\-(\\d+)", tfPrefix))
+var tagRegex = regexp.MustCompile(fmt.Sprintf("%s\\-(\\d+)", TFLockTagPrefix))
 
 func (a TriggerAction) String() string {
 	switch a {
@@ -225,6 +225,23 @@ func FindLockingMR(ctx context.Context, tags []string, thisMR string) string {
 	return ""
 }
 
+// buildLockingMRURL constructs the full MR URL for a locking MR by replacing
+// the IID at the end of the current MR's URL with the locking MR's IID.
+// e.g. currentMRWebURL="https://gitlab.com/org/repo/-/merge_requests/100",
+//
+//	lockingMRIID="999" → "https://gitlab.com/org/repo/-/merge_requests/999"
+func buildLockingMRURL(currentMRWebURL, lockingMRIID string) string {
+	if currentMRWebURL == "" {
+		log.Warn().Str("lockingMRIID", lockingMRIID).Msg("buildLockingMRURL: currentMRWebURL is empty, returning bare MR IID")
+		return lockingMRIID
+	}
+	idx := strings.LastIndex(currentMRWebURL, "/")
+	if idx < 0 {
+		return lockingMRIID
+	}
+	return currentMRWebURL[:idx+1] + lockingMRIID
+}
+
 // handleError both logs an error and reports it back to the Merge Request via an MR comment.
 // the returned error is identical to the input parameter as a convenience
 func (t *TFCTrigger) handleError(ctx context.Context, err error, msg string) error {
@@ -249,7 +266,7 @@ func (t *TFCTrigger) getLockingMR(ctx context.Context, workspace string) string
 	ctx, span := otel.Tracer("TFC").Start(ctx, "getLockingMR")
 	defer span.End()
 
-	tags, err := t.tfc.GetTagsByQuery(ctx, workspace, tfPrefix)
+	tags, err := t.tfc.GetTagsByQuery(ctx, workspace, TFLockTagPrefix)
 	if err != nil {
 		log.Error().Err(err)
 	}
@@ -458,7 +475,7 @@ func (t *TFCTrigger) TriggerCleanupEvent(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("ignoring cleanup trigger for project, missing .tfbuddy.yaml. %w", err)
 	}
-	tag := fmt.Sprintf("%s-%d", tfPrefix, mr.GetInternalID())
+	tag := fmt.Sprintf("%s-%d", TFLockTagPrefix, mr.GetInternalID())
 	for _, cfgWS := range cfg.Workspaces {
 		ws, err := t.tfc.GetWorkspaceByName(ctx,
 			cfgWS.Organization,
@@ -563,18 +580,23 @@ func (t *TFCTrigger) triggerRunForWorkspace(ctx context.Context, cfgWS *TFCWorks
 	} else if t.GetAction() != PlanAction {
 		return fmt.Errorf("run action was not apply or plan. %w", err)
 	}
-	// If the workspace is locked tell the user and don't queue a run
-	// Otherwise, TFC wil queue an apply, which might put them out of order
+	// If the workspace is locked tell the user and don't queue a run.
+	// Otherwise, TFC will queue an apply, which might put them out of order.
 	if isApply {
 		lockingMR := t.getLockingMR(ctx, ws.ID)
 		if ws.Locked {
-			return fmt.Errorf("refusing to Apply changes to a locked workspace. %w", err)
+			if lockingMR != "" {
+				lockingMRURL := buildLockingMRURL(mr.GetWebURL(), lockingMR)
+				return fmt.Errorf("refusing to Apply changes to a locked workspace. Lock is held by MR: %s", lockingMRURL)
+			}
+			return fmt.Errorf("refusing to Apply changes to a locked workspace. Check the TFC workspace UI for lock details.")
 		} else if lockingMR != "" {
-			return fmt.Errorf("workspace is locked by another MR! %s", lockingMR)
+			lockingMRURL := buildLockingMRURL(mr.GetWebURL(), lockingMR)
+			return fmt.Errorf("refusing to Apply changes to a locked workspace. Lock is held by another MR: %s", lockingMRURL)
 		} else {
 			err = t.tfc.AddTags(ctx,
 				ws.ID,
-				tfPrefix,
+				TFLockTagPrefix,
 				fmt.Sprintf("%d", t.GetMergeRequestIID()),
 			)
 			if err != nil {

pkg/tfc_trigger/tfc_trigger_test.go

@@ -807,6 +807,157 @@ func TestAutoMerge_Globally_Disabled(t *testing.T) {
 	}
 }
 
+func TestTFCEvents_ApplyBlockedByTFCLockWithLockingMR(t *testing.T) {
+	ws := &tfc_trigger.ProjectConfig{
+		Workspaces: []*tfc_trigger.TFCWorkspace{{
+			Name:         "service-tfbuddy",
+			Organization: "zapier-test",
+			Mode:         "apply-before-merge",
+		}}}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+	testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t)
+
+	// Register BEFORE InitTestSuite — first AnyTimes() wins in FIFO order.
+	testSuite.MockApiClient.EXPECT().
+		GetWorkspaceByName(gomock.Any(), gomock.Any(), gomock.Any()).
+		DoAndReturn(func(ctx context.Context, org, name string) (*tfe.Workspace, error) {
+			return &tfe.Workspace{ID: name, Locked: true}, nil
+		}).AnyTimes()
+	testSuite.MockApiClient.EXPECT().
+		GetTagsByQuery(gomock.Any(), gomock.Any(), "tfbuddylock").
+		Return([]string{"tfbuddylock-999"}, nil).AnyTimes()
+	testSuite.MockGitMR.EXPECT().
+		GetWebURL().
+		Return("https://gitlab.com/zapier/tfbuddy/-/merge_requests/101").AnyTimes()
+
+	testSuite.InitTestSuite()
+
+	tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{
+		Action:                   tfc_trigger.ApplyAction,
+		Branch:                   "test-branch",
+		CommitSHA:                "abcd12233",
+		ProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS,
+		MergeRequestIID:          testSuite.MetaData.MRIID,
+		TriggerSource:            tfc_trigger.CommentTrigger,
+	})
+	trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg)
+	ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST")
+	triggeredWS, err := trigger.TriggerTFCEvents(ctx)
+
+	if err != nil {
+		t.Fatal("expected no fatal error, got:", err)
+	}
+	if len(triggeredWS.Errored) == 0 {
+		t.Fatal("expected workspace to be in errored list")
+	}
+	wantURL := "https://gitlab.com/zapier/tfbuddy/-/merge_requests/999"
+	if !strings.Contains(triggeredWS.Errored[0].Error, wantURL) {
+		t.Fatalf("expected error to contain %q, got: %s", wantURL, triggeredWS.Errored[0].Error)
+	}
+}
+
+func TestTFCEvents_ApplyBlockedByTFCLockNoTag(t *testing.T) {
+	ws := &tfc_trigger.ProjectConfig{
+		Workspaces: []*tfc_trigger.TFCWorkspace{{
+			Name:         "service-tfbuddy",
+			Organization: "zapier-test",
+			Mode:         "apply-before-merge",
+		}}}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+	testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t)
+
+	testSuite.MockApiClient.EXPECT().
+		GetWorkspaceByName(gomock.Any(), gomock.Any(), gomock.Any()).
+		DoAndReturn(func(ctx context.Context, org, name string) (*tfe.Workspace, error) {
+			return &tfe.Workspace{ID: name, Locked: true}, nil
+		}).AnyTimes()
+	testSuite.MockApiClient.EXPECT().
+		GetTagsByQuery(gomock.Any(), gomock.Any(), "tfbuddylock").
+		Return([]string{}, nil).AnyTimes()
+
+	testSuite.InitTestSuite()
+
+	tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{
+		Action:                   tfc_trigger.ApplyAction,
+		Branch:                   "test-branch",
+		CommitSHA:                "abcd12233",
+		ProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS,
+		MergeRequestIID:          testSuite.MetaData.MRIID,
+		TriggerSource:            tfc_trigger.CommentTrigger,
+	})
+	trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg)
+	ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST")
+	triggeredWS, err := trigger.TriggerTFCEvents(ctx)
+
+	if err != nil {
+		t.Fatal("expected no fatal error, got:", err)
+	}
+	if len(triggeredWS.Errored) == 0 {
+		t.Fatal("expected workspace to be in errored list")
+	}
+	wantSubstr := "refusing to Apply changes to a locked workspace"
+	if !strings.Contains(triggeredWS.Errored[0].Error, wantSubstr) {
+		t.Fatalf("expected error to contain %q, got: %s", wantSubstr, triggeredWS.Errored[0].Error)
+	}
+	if strings.Contains(triggeredWS.Errored[0].Error, "merge_requests") {
+		t.Fatalf("error should not reference merge_requests when no tag exists, got: %s", triggeredWS.Errored[0].Error)
+	}
+}
+
+func TestTFCEvents_ApplyBlockedByTagLock(t *testing.T) {
+	ws := &tfc_trigger.ProjectConfig{
+		Workspaces: []*tfc_trigger.TFCWorkspace{{
+			Name:         "service-tfbuddy",
+			Organization: "zapier-test",
+			Mode:         "apply-before-merge",
+		}}}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+	testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t)
+
+	testSuite.MockApiClient.EXPECT().
+		GetWorkspaceByName(gomock.Any(), gomock.Any(), gomock.Any()).
+		DoAndReturn(func(ctx context.Context, org, name string) (*tfe.Workspace, error) {
+			return &tfe.Workspace{ID: name, Locked: false}, nil
+		}).AnyTimes()
+	testSuite.MockApiClient.EXPECT().
+		GetTagsByQuery(gomock.Any(), gomock.Any(), "tfbuddylock").
+		Return([]string{"tfbuddylock-999"}, nil).AnyTimes()
+	testSuite.MockGitMR.EXPECT().
+		GetWebURL().
+		Return("https://gitlab.com/zapier/tfbuddy/-/merge_requests/101").AnyTimes()
+
+	testSuite.InitTestSuite()
+
+	tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{
+		Action:                   tfc_trigger.ApplyAction,
+		Branch:                   "test-branch",
+		CommitSHA:                "abcd12233",
+		ProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS,
+		MergeRequestIID:          testSuite.MetaData.MRIID,
+		TriggerSource:            tfc_trigger.CommentTrigger,
+	})
+	trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg)
+	ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST")
+	triggeredWS, err := trigger.TriggerTFCEvents(ctx)
+
+	if err != nil {
+		t.Fatal("expected no fatal error, got:", err)
+	}
+	if len(triggeredWS.Errored) == 0 {
+		t.Fatal("expected workspace to be in errored list")
+	}
+	wantURL := "https://gitlab.com/zapier/tfbuddy/-/merge_requests/999"
+	if !strings.Contains(triggeredWS.Errored[0].Error, wantURL) {
+		t.Fatalf("expected error to contain %q, got: %s", wantURL, triggeredWS.Errored[0].Error)
+	}
+}
+
 // TestTFCEvents_ApplyNotBlockedByDifferentServiceChanges verifies that changes
 // on the target branch in a different service directory do not block applies for
 // unrelated workspaces.

pkg/vcs/gitlab/mr_comment_updater.go

@@ -11,12 +11,42 @@ import (
 	"github.com/rs/zerolog/log"
 
 	"github.com/zapier/tfbuddy/pkg/runstream"
+	"github.com/zapier/tfbuddy/pkg/tfc_trigger"
 )
 
+// releaseWorkspaceLockTag removes the TFBuddy tag-based workspace lock when an
+// apply-triggered run reaches a terminal state. This prevents stale lock tags
+// from blocking future applies after a run finishes while the MR stays open.
+func (p *RunStatusUpdater) releaseWorkspaceLockTag(ctx context.Context, run *tfe.Run, rmd runstream.RunMetadata) {
+	if rmd.GetAction() != runstream.ApplyAction {
+		return
+	}
+	// run.Workspace is always populated by GetRun (includes tfe.RunWorkspace).
+	switch run.Status {
+	case tfe.RunApplied, tfe.RunErrored, tfe.RunCanceled, tfe.RunDiscarded:
+		tag := fmt.Sprintf("%s-%d", tfc_trigger.TFLockTagPrefix, rmd.GetMRInternalID())
+		if err := p.tfc.RemoveTagsByQuery(ctx, run.Workspace.ID, tag); err != nil {
+			log.Error().Err(err).
+				Str("workspace", run.Workspace.Name).
+				Str("workspaceID", run.Workspace.ID).
+				Int("mrIID", rmd.GetMRInternalID()).
+				Msg("could not remove workspace lock tag after apply completion")
+		} else {
+			log.Info().
+				Str("workspace", run.Workspace.Name).
+				Int("mrIID", rmd.GetMRInternalID()).
+				Str("runStatus", string(run.Status)).
+				Msg("released workspace lock tag after apply reached terminal state")
+		}
+	}
+}
+
 func (p *RunStatusUpdater) postRunStatusComment(ctx context.Context, run *tfe.Run, rmd runstream.RunMetadata) {
 	ctx, span := otel.Tracer("TFC").Start(ctx, "postRunStatusComment")
 	defer span.End()
 
+	p.releaseWorkspaceLockTag(ctx, run, rmd)
+
 	commentBody, topLevelNoteBody, resolveDiscussion := comment_formatter.FormatRunStatusCommentBody(p.tfc, run, rmd)
 
 	var oldUrls string