fix: throw error when there is a duplicate workload name (#1395)

Co-authored-by: taskbot <[email protected]>

Author: yrobla

cmd/thv/app/run.go

@@ -149,6 +149,13 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to create workload manager: %v", err)
 	}
 
+	exists, err := workloadManager.DoesWorkloadExist(ctx, runFlags.Name)
+	if err != nil {
+		return fmt.Errorf("failed to check if workload exists: %v", err)
+	}
+	if exists {
+		return fmt.Errorf("workload with name '%s' already exists", runFlags.Name)
+	}
 	err = validateGroup(ctx, workloadManager, serverOrImage)
 	if err != nil {
 		return err

pkg/api/v1/workloads.go

@@ -275,6 +275,17 @@ func (s *WorkloadRoutes) createWorkload(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
+	// check if the workload already exists
+	exists, err := s.workloadManager.DoesWorkloadExist(ctx, req.Name)
+	if err != nil {
+		http.Error(w, fmt.Sprintf("Failed to check if workload exists: %v", err), http.StatusInternalServerError)
+		return
+	}
+	if exists {
+		http.Error(w, fmt.Sprintf("Workload with name %s already exists", req.Name), http.StatusConflict)
+		return
+	}
+
 	// NOTE: None of the k8s-related config logic is included here.
 	runSecrets := secrets.SecretParametersToCLI(req.Secrets)
 	runConfig, err := runner.NewRunConfigBuilder().

pkg/workloads/manager.go

@@ -62,6 +62,8 @@ type Manager interface {
 	MoveToDefaultGroup(ctx context.Context, workloadNames []string, groupName string) error
 	// ListWorkloadsInGroup returns all workload names that belong to the specified group, including stopped workloads.
 	ListWorkloadsInGroup(ctx context.Context, groupName string) ([]string, error)
+	// DoesWorkloadExist checks if a workload with the given name exists.
+	DoesWorkloadExist(ctx context.Context, workloadName string) (bool, error)
 }
 
 type defaultManager struct {
@@ -114,6 +116,23 @@ func (d *defaultManager) GetWorkload(ctx context.Context, workloadName string) (
 	return d.statuses.GetWorkload(ctx, workloadName)
 }
 
+func (d *defaultManager) DoesWorkloadExist(ctx context.Context, workloadName string) (bool, error) {
+	// check if workload exists by trying to get it
+	workload, err := d.statuses.GetWorkload(ctx, workloadName)
+	if err != nil {
+		if errors.Is(err, rt.ErrWorkloadNotFound) {
+			return false, nil
+		}
+		return false, fmt.Errorf("failed to check if workload exists: %w", err)
+	}
+
+	// now check if the workload is not in error
+	if workload.Status == rt.WorkloadStatusError {
+		return false, nil
+	}
+	return true, nil
+}
+
 func (d *defaultManager) ListWorkloads(ctx context.Context, listAll bool, labelFilters ...string) ([]core.Workload, error) {
 	// For the sake of minimizing changes, delegate to the status manager.
 	// Whether this method should still belong to the workload manager is TBD.

pkg/workloads/mocks/mock_manager.go

@@ -408,4 +408,43 @@ var _ = Describe("OsvMcpServer", Serial, func() {
 			})
 		})
 	})
+
+	Describe("We cannot create duplicate servers", func() {
+		It("should reject starting a second workload with the same name [Serial]", func() {
+			// unique name for this test
+			serverName := generateUniqueServerName("osv-duplicate-name-test")
+
+			By("Starting the first OSV MCP server")
+			e2e.NewTHVCommand(config, "run",
+				"--name", serverName,
+				"--transport", "sse", "osv").ExpectSuccess()
+
+			// ensure it's actually up before attempting the duplicate
+			err := e2e.WaitForMCPServer(config, serverName, 60*time.Second)
+			Expect(err).ToNot(HaveOccurred(), "first server should start")
+
+			By("Attempting to start a second server with the same name")
+			// Use Run() (not ExpectSuccess) so we can assert failure +
+			// examine stdout/stderr
+			stdout, stderr, runErr := e2e.NewTHVCommand(config, "run",
+				"--name", serverName,
+				"--transport", "sse",
+				"osv").Run()
+
+			// The second run must fail because the name already exists
+			Expect(runErr).To(HaveOccurred(), "second server with same name should fail")
+			// Be flexible on the exact message, but check for a helpful hint
+			Expect(stdout+stderr).To(
+				ContainSubstring("already exists"),
+				"CLI should report a duplicate-name conflict",
+			)
+
+			// Cleanup
+			if config.CleanupAfter {
+				cerr := e2e.StopAndRemoveMCPServer(config, serverName)
+				Expect(cerr).ToNot(HaveOccurred(), "cleanup should succeed")
+			}
+		})
+
+	})
 })