From 17f4847dc302b4e3ebcb19437dcc242a94827492 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 12 Mar 2026 14:14:04 +0100 Subject: [PATCH 1/5] feat(instances): integrate runtime management with instance manager Instances can now have associated runtimes that manage their lifecycle. The manager handles runtime registration, instance creation with runtimes, and starting/stopping runtime instances. Context is propagated through all runtime operations to support cancellation and timeouts. Co-Authored-By: Claude Code (Claude Sonnet 4.5) Signed-off-by: Philippe Martin --- pkg/cmd/init.go | 12 +- pkg/cmd/workspace_list_test.go | 32 +- pkg/cmd/workspace_remove.go | 2 +- pkg/cmd/workspace_remove_test.go | 66 +++- pkg/instances/instance.go | 40 +++ pkg/instances/instance_runtime_test.go | 204 ++++++++++++ pkg/instances/manager.go | 253 ++++++++++++-- pkg/instances/manager_test.go | 436 +++++++++++++++++++++---- 8 files changed, 944 insertions(+), 101 deletions(-) create mode 100644 pkg/instances/instance_runtime_test.go diff --git a/pkg/cmd/init.go b/pkg/cmd/init.go index 948d7e8..3a15a9e 100644 --- a/pkg/cmd/init.go +++ b/pkg/cmd/init.go @@ -25,6 +25,7 @@ import ( "path/filepath" "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/kortex-hub/kortex-cli/pkg/runtime/fake" "github.com/spf13/cobra" ) @@ -71,6 +72,13 @@ func (i *initCmd) preRun(cmd *cobra.Command, args []string) error { if err != nil { return outputErrorIfJSON(cmd, i.output, fmt.Errorf("failed to create manager: %w", err)) } + + // Register fake runtime (for testing) + // TODO: In production, register only the runtimes that are available/configured + if err := manager.RegisterRuntime(fake.New()); err != nil { + return fmt.Errorf("failed to register fake runtime: %w", err) + } + i.manager = manager // Get sources directory (default to current directory) @@ -124,8 +132,8 @@ func (i *initCmd) run(cmd *cobra.Command, args []string) error { return outputErrorIfJSON(cmd, i.output, err) } - // Add the instance to the manager - addedInstance, err := i.manager.Add(instance) + // Add the instance to the manager with runtime + addedInstance, err := i.manager.Add(cmd.Context(), instance, "fake") if err != nil { return outputErrorIfJSON(cmd, i.output, err) } diff --git a/pkg/cmd/workspace_list_test.go b/pkg/cmd/workspace_list_test.go index cec5d65..28f656b 100644 --- a/pkg/cmd/workspace_list_test.go +++ b/pkg/cmd/workspace_list_test.go @@ -20,6 +20,7 @@ package cmd import ( "bytes" + "context" "encoding/json" "os" "path/filepath" @@ -29,6 +30,7 @@ import ( api "github.com/kortex-hub/kortex-cli-api/cli/go" "github.com/kortex-hub/kortex-cli/pkg/cmd/testutil" "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/kortex-hub/kortex-cli/pkg/runtime/fake" "github.com/spf13/cobra" ) @@ -243,7 +245,12 @@ func TestWorkspaceListCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance: %v", err) } - addedInstance, err := manager.Add(instance) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") if err != nil { t.Fatalf("Failed to add instance: %v", err) } @@ -308,12 +315,17 @@ func TestWorkspaceListCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance 2: %v", err) } - addedInstance1, err := manager.Add(instance1) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance1, err := manager.Add(context.Background(), instance1, "fake") if err != nil { t.Fatalf("Failed to add instance 1: %v", err) } - addedInstance2, err := manager.Add(instance2) + addedInstance2, err := manager.Add(context.Background(), instance2, "fake") if err != nil { t.Fatalf("Failed to add instance 2: %v", err) } @@ -385,7 +397,12 @@ func TestWorkspaceListCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance: %v", err) } - addedInstance, err := manager.Add(instance) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") if err != nil { t.Fatalf("Failed to add instance: %v", err) } @@ -473,7 +490,12 @@ func TestWorkspaceListCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance: %v", err) } - addedInstance, err := manager.Add(instance) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") if err != nil { t.Fatalf("Failed to add instance: %v", err) } diff --git a/pkg/cmd/workspace_remove.go b/pkg/cmd/workspace_remove.go index 4ef8158..0227fff 100644 --- a/pkg/cmd/workspace_remove.go +++ b/pkg/cmd/workspace_remove.go @@ -77,7 +77,7 @@ func (w *workspaceRemoveCmd) preRun(cmd *cobra.Command, args []string) error { // run executes the workspace remove command logic func (w *workspaceRemoveCmd) run(cmd *cobra.Command, args []string) error { // Delete the instance - err := w.manager.Delete(w.id) + err := w.manager.Delete(cmd.Context(), w.id) if err != nil { if errors.Is(err, instances.ErrInstanceNotFound) { if w.output == "json" { diff --git a/pkg/cmd/workspace_remove_test.go b/pkg/cmd/workspace_remove_test.go index 29704a2..dfabc18 100644 --- a/pkg/cmd/workspace_remove_test.go +++ b/pkg/cmd/workspace_remove_test.go @@ -20,6 +20,7 @@ package cmd import ( "bytes" + "context" "encoding/json" "os" "path/filepath" @@ -29,6 +30,7 @@ import ( api "github.com/kortex-hub/kortex-cli-api/cli/go" "github.com/kortex-hub/kortex-cli/pkg/cmd/testutil" "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/kortex-hub/kortex-cli/pkg/runtime/fake" "github.com/spf13/cobra" ) @@ -220,6 +222,46 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { } }) + t.Run("creates manager from storage flag", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + sourcesDir := t.TempDir() + + // Create a workspace first + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instance, err := instances.NewInstance(instances.NewInstanceParams{ + SourceDir: sourcesDir, + ConfigDir: filepath.Join(sourcesDir, ".kortex"), + }) + if err != nil { + t.Fatalf("Failed to create instance: %v", err) + } + + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") + if err != nil { + t.Fatalf("Failed to add instance: %v", err) + } + + // Now remove it + rootCmd := NewRootCmd() + rootCmd.SetArgs([]string{"workspace", "remove", addedInstance.GetID(), "--storage", storageDir}) + + err = rootCmd.Execute() + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + }) + t.Run("removes existing workspace by ID", func(t *testing.T) { t.Parallel() @@ -240,7 +282,12 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance: %v", err) } - addedInstance, err := manager.Add(instance) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") if err != nil { t.Fatalf("Failed to add instance: %v", err) } @@ -308,6 +355,7 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Run("removes only specified workspace when multiple exist", func(t *testing.T) { t.Parallel() + ctx := context.Background() storageDir := t.TempDir() sourcesDir1 := t.TempDir() sourcesDir2 := t.TempDir() @@ -334,12 +382,17 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance 2: %v", err) } - addedInstance1, err := manager.Add(instance1) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance1, err := manager.Add(ctx, instance1, "fake") if err != nil { t.Fatalf("Failed to add instance 1: %v", err) } - addedInstance2, err := manager.Add(instance2) + addedInstance2, err := manager.Add(ctx, instance2, "fake") if err != nil { t.Fatalf("Failed to add instance 2: %v", err) } @@ -404,7 +457,12 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance: %v", err) } - addedInstance, err := manager.Add(instance) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") if err != nil { t.Fatalf("Failed to add instance: %v", err) } diff --git a/pkg/instances/instance.go b/pkg/instances/instance.go index 8d30670..bbe8b5b 100644 --- a/pkg/instances/instance.go +++ b/pkg/instances/instance.go @@ -16,6 +16,7 @@ package instances import ( "errors" + "maps" "os" "path/filepath" ) @@ -37,6 +38,18 @@ type InstancePaths struct { Configuration string `json:"configuration"` } +// RuntimeData represents runtime-specific information for an instance +type RuntimeData struct { + // Type is the runtime type (e.g., "fake", "podman", "docker") + Type string `json:"type"` + // InstanceID is the runtime-assigned instance identifier + InstanceID string `json:"instance_id"` + // State is the current runtime state (e.g., "created", "running", "stopped") + State string `json:"state"` + // Info contains runtime-specific metadata + Info map[string]string `json:"info"` +} + // InstanceData represents the serializable data of an instance type InstanceData struct { // ID is the unique identifier for the instance @@ -45,6 +58,8 @@ type InstanceData struct { Name string `json:"name"` // Paths contains the source and configuration directories Paths InstancePaths `json:"paths"` + // Runtime contains runtime-specific information + Runtime RuntimeData `json:"runtime"` } // Instance represents a workspace instance with source and configuration directories. @@ -63,6 +78,10 @@ type Instance interface { GetConfigDir() string // IsAccessible checks if both source and config directories are accessible IsAccessible() bool + // GetRuntimeType returns the runtime type for this instance + GetRuntimeType() string + // GetRuntimeData returns the complete runtime data for this instance + GetRuntimeData() RuntimeData // Dump returns the serializable data of the instance Dump() InstanceData } @@ -79,6 +98,8 @@ type instance struct { // ConfigDir is the directory containing workspace configuration. // This is always stored as an absolute path. ConfigDir string + // Runtime contains runtime-specific information + Runtime RuntimeData } // Compile-time check to ensure instance implements Instance interface @@ -115,6 +136,23 @@ func (i *instance) IsAccessible() bool { return true } +// GetRuntimeType returns the runtime type for this instance +func (i *instance) GetRuntimeType() string { + return i.Runtime.Type +} + +// GetRuntimeData returns the complete runtime data for this instance +func (i *instance) GetRuntimeData() RuntimeData { + infoCopy := make(map[string]string, len(i.Runtime.Info)) + maps.Copy(infoCopy, i.Runtime.Info) + return RuntimeData{ + Type: i.Runtime.Type, + InstanceID: i.Runtime.InstanceID, + State: i.Runtime.State, + Info: infoCopy, + } +} + // Dump returns the serializable data of the instance func (i *instance) Dump() InstanceData { return InstanceData{ @@ -124,6 +162,7 @@ func (i *instance) Dump() InstanceData { Source: i.SourceDir, Configuration: i.ConfigDir, }, + Runtime: i.Runtime, } } @@ -188,6 +227,7 @@ func NewInstanceFromData(data InstanceData) (Instance, error) { Name: data.Name, SourceDir: data.Paths.Source, ConfigDir: data.Paths.Configuration, + Runtime: data.Runtime, }, nil } diff --git a/pkg/instances/instance_runtime_test.go b/pkg/instances/instance_runtime_test.go new file mode 100644 index 0000000..41d3a2e --- /dev/null +++ b/pkg/instances/instance_runtime_test.go @@ -0,0 +1,204 @@ +// Copyright 2026 Red Hat, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package instances + +import ( + "encoding/json" + "path/filepath" + "testing" +) + +func TestInstanceData_SerializeWithRuntime(t *testing.T) { + t.Parallel() + + data := InstanceData{ + ID: "test-id", + Name: "test-instance", + Paths: InstancePaths{ + Source: "/path/to/source", + Configuration: "/path/to/config", + }, + Runtime: RuntimeData{ + Type: "fake", + InstanceID: "fake-001", + State: "running", + Info: map[string]string{ + "created_at": "2026-03-11T10:00:00Z", + }, + }, + } + + // Serialize to JSON + jsonBytes, err := json.Marshal(data) + if err != nil { + t.Fatalf("Failed to marshal InstanceData: %v", err) + } + + // Deserialize back + var deserialized InstanceData + err = json.Unmarshal(jsonBytes, &deserialized) + if err != nil { + t.Fatalf("Failed to unmarshal InstanceData: %v", err) + } + + // Verify all fields + if deserialized.ID != data.ID { + t.Errorf("ID = %v, want %v", deserialized.ID, data.ID) + } + if deserialized.Name != data.Name { + t.Errorf("Name = %v, want %v", deserialized.Name, data.Name) + } + if deserialized.Paths.Source != data.Paths.Source { + t.Errorf("Paths.Source = %v, want %v", deserialized.Paths.Source, data.Paths.Source) + } + if deserialized.Paths.Configuration != data.Paths.Configuration { + t.Errorf("Paths.Configuration = %v, want %v", deserialized.Paths.Configuration, data.Paths.Configuration) + } + if deserialized.Runtime.Type != data.Runtime.Type { + t.Errorf("Runtime.Type = %v, want %v", deserialized.Runtime.Type, data.Runtime.Type) + } + if deserialized.Runtime.InstanceID != data.Runtime.InstanceID { + t.Errorf("Runtime.InstanceID = %v, want %v", deserialized.Runtime.InstanceID, data.Runtime.InstanceID) + } + if deserialized.Runtime.State != data.Runtime.State { + t.Errorf("Runtime.State = %v, want %v", deserialized.Runtime.State, data.Runtime.State) + } + if deserialized.Runtime.Info["created_at"] != data.Runtime.Info["created_at"] { + t.Errorf("Runtime.Info[created_at] = %v, want %v", deserialized.Runtime.Info["created_at"], data.Runtime.Info["created_at"]) + } +} + +func TestInstance_GetRuntimeType(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + data := InstanceData{ + ID: "test-id", + Name: "test-instance", + Paths: InstancePaths{ + Source: filepath.Join(tmpDir, "source"), + Configuration: filepath.Join(tmpDir, "config"), + }, + Runtime: RuntimeData{ + Type: "fake", + InstanceID: "fake-001", + State: "running", + }, + } + + inst, err := NewInstanceFromData(data) + if err != nil { + t.Fatalf("NewInstanceFromData() failed: %v", err) + } + + if inst.GetRuntimeType() != "fake" { + t.Errorf("GetRuntimeType() = %v, want 'fake'", inst.GetRuntimeType()) + } +} + +func TestInstance_GetRuntimeData(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + runtimeData := RuntimeData{ + Type: "fake", + InstanceID: "fake-001", + State: "running", + Info: map[string]string{ + "created_at": "2026-03-11T10:00:00Z", + "started_at": "2026-03-11T10:01:00Z", + }, + } + + data := InstanceData{ + ID: "test-id", + Name: "test-instance", + Paths: InstancePaths{ + Source: filepath.Join(tmpDir, "source"), + Configuration: filepath.Join(tmpDir, "config"), + }, + Runtime: runtimeData, + } + + inst, err := NewInstanceFromData(data) + if err != nil { + t.Fatalf("NewInstanceFromData() failed: %v", err) + } + + info := inst.GetRuntimeData() + + if info.Type != runtimeData.Type { + t.Errorf("GetRuntimeData().Type = %v, want %v", info.Type, runtimeData.Type) + } + if info.InstanceID != runtimeData.InstanceID { + t.Errorf("GetRuntimeData().InstanceID = %v, want %v", info.InstanceID, runtimeData.InstanceID) + } + if info.State != runtimeData.State { + t.Errorf("GetRuntimeData().State = %v, want %v", info.State, runtimeData.State) + } + if info.Info["created_at"] != runtimeData.Info["created_at"] { + t.Errorf("GetRuntimeData().Info[created_at] = %v, want %v", info.Info["created_at"], runtimeData.Info["created_at"]) + } + if info.Info["started_at"] != runtimeData.Info["started_at"] { + t.Errorf("GetRuntimeData().Info[started_at] = %v, want %v", info.Info["started_at"], runtimeData.Info["started_at"]) + } +} + +func TestInstance_DumpIncludesRuntime(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + runtimeData := RuntimeData{ + Type: "fake", + InstanceID: "fake-001", + State: "running", + Info: map[string]string{ + "key": "value", + }, + } + + data := InstanceData{ + ID: "test-id", + Name: "test-instance", + Paths: InstancePaths{ + Source: filepath.Join(tmpDir, "source"), + Configuration: filepath.Join(tmpDir, "config"), + }, + Runtime: runtimeData, + } + + inst, err := NewInstanceFromData(data) + if err != nil { + t.Fatalf("NewInstanceFromData() failed: %v", err) + } + + dumped := inst.Dump() + + if dumped.Runtime.Type != runtimeData.Type { + t.Errorf("Dump().Runtime.Type = %v, want %v", dumped.Runtime.Type, runtimeData.Type) + } + if dumped.Runtime.InstanceID != runtimeData.InstanceID { + t.Errorf("Dump().Runtime.InstanceID = %v, want %v", dumped.Runtime.InstanceID, runtimeData.InstanceID) + } + if dumped.Runtime.State != runtimeData.State { + t.Errorf("Dump().Runtime.State = %v, want %v", dumped.Runtime.State, runtimeData.State) + } + if dumped.Runtime.Info["key"] != runtimeData.Info["key"] { + t.Errorf("Dump().Runtime.Info[key] = %v, want %v", dumped.Runtime.Info["key"], runtimeData.Info["key"]) + } +} diff --git a/pkg/instances/manager.go b/pkg/instances/manager.go index 0ac3fbf..5f10b3d 100644 --- a/pkg/instances/manager.go +++ b/pkg/instances/manager.go @@ -15,6 +15,7 @@ package instances import ( + "context" "encoding/json" "errors" "fmt" @@ -23,6 +24,7 @@ import ( "sync" "github.com/kortex-hub/kortex-cli/pkg/generator" + "github.com/kortex-hub/kortex-cli/pkg/runtime" ) const ( @@ -35,25 +37,32 @@ type InstanceFactory func(InstanceData) (Instance, error) // Manager handles instance storage and operations type Manager interface { - // Add registers a new instance and returns the instance with its generated ID - Add(inst Instance) (Instance, error) + // Add registers a new instance with a runtime and returns the instance with its generated ID + Add(ctx context.Context, inst Instance, runtimeType string) (Instance, error) + // Start starts a runtime instance by ID + Start(ctx context.Context, id string) error + // Stop stops a runtime instance by ID + Stop(ctx context.Context, id string) error // List returns all registered instances List() ([]Instance, error) // Get retrieves a specific instance by ID Get(id string) (Instance, error) // Delete unregisters an instance by ID - Delete(id string) error + Delete(ctx context.Context, id string) error // Reconcile removes instances with inaccessible directories // Returns the list of removed instance IDs Reconcile() ([]string, error) + // RegisterRuntime registers a runtime with the manager's registry + RegisterRuntime(rt runtime.Runtime) error } // manager is the internal implementation of Manager type manager struct { - storageFile string - mu sync.RWMutex - factory InstanceFactory - generator generator.Generator + storageFile string + mu sync.RWMutex + factory InstanceFactory + generator generator.Generator + runtimeRegistry runtime.Registry } // Compile-time check to ensure manager implements Manager interface @@ -61,12 +70,12 @@ var _ Manager = (*manager)(nil) // NewManager creates a new instance manager with the given storage directory. func NewManager(storageDir string) (Manager, error) { - return newManagerWithFactory(storageDir, NewInstanceFromData, generator.New()) + return newManagerWithFactory(storageDir, NewInstanceFromData, generator.New(), runtime.NewRegistry()) } -// newManagerWithFactory creates a new instance manager with a custom instance factory and generator. -// This is unexported and primarily useful for testing with fake instances and generators. -func newManagerWithFactory(storageDir string, factory InstanceFactory, gen generator.Generator) (Manager, error) { +// newManagerWithFactory creates a new instance manager with a custom instance factory, generator, and registry. +// This is unexported and primarily useful for testing with fake instances, generators, and runtimes. +func newManagerWithFactory(storageDir string, factory InstanceFactory, gen generator.Generator, reg runtime.Registry) (Manager, error) { if storageDir == "" { return nil, errors.New("storage directory cannot be empty") } @@ -76,6 +85,9 @@ func newManagerWithFactory(storageDir string, factory InstanceFactory, gen gener if gen == nil { return nil, errors.New("generator cannot be nil") } + if reg == nil { + return nil, errors.New("registry cannot be nil") + } // Ensure storage directory exists if err := os.MkdirAll(storageDir, 0755); err != nil { @@ -84,21 +96,26 @@ func newManagerWithFactory(storageDir string, factory InstanceFactory, gen gener storageFile := filepath.Join(storageDir, DefaultStorageFileName) return &manager{ - storageFile: storageFile, - factory: factory, - generator: gen, + storageFile: storageFile, + factory: factory, + generator: gen, + runtimeRegistry: reg, }, nil } -// Add registers a new instance. +// Add registers a new instance with a runtime. // The instance must be created using NewInstance to ensure proper validation. // A unique ID is generated for the instance when it's added to storage. // If the instance name is empty, a unique name is generated from the source directory. -// Returns the instance with its generated ID and name. -func (m *manager) Add(inst Instance) (Instance, error) { +// The runtime instance is created but not started. +// Returns the instance with its generated ID, name, and runtime information. +func (m *manager) Add(ctx context.Context, inst Instance, runtimeType string) (Instance, error) { if inst == nil { return nil, errors.New("instance cannot be nil") } + if runtimeType == "" { + return nil, errors.New("runtime type cannot be empty") + } m.mu.Lock() defer m.mu.Unlock() @@ -134,22 +151,179 @@ func (m *manager) Add(inst Instance) (Instance, error) { name = m.ensureUniqueName(name, instances) } - // Create a new instance with the unique ID and name + // Get the runtime + rt, err := m.runtimeRegistry.Get(runtimeType) + if err != nil { + return nil, fmt.Errorf("failed to get runtime: %w", err) + } + + // Create runtime instance + runtimeInfo, err := rt.Create(ctx, runtime.CreateParams{ + Name: name, + SourcePath: inst.GetSourceDir(), + ConfigPath: inst.GetConfigDir(), + }) + if err != nil { + return nil, fmt.Errorf("failed to create runtime instance: %w", err) + } + + // Create a new instance with the unique ID, name, and runtime info instanceWithID := &instance{ ID: uniqueID, Name: name, SourceDir: inst.GetSourceDir(), ConfigDir: inst.GetConfigDir(), + Runtime: RuntimeData{ + Type: runtimeType, + InstanceID: runtimeInfo.ID, + State: runtimeInfo.State, + Info: runtimeInfo.Info, + }, } instances = append(instances, instanceWithID) if err := m.saveInstances(instances); err != nil { + // Cleanup: try to remove the runtime instance + removeErr := rt.Remove(ctx, runtimeInfo.ID) + if removeErr != nil { + return nil, fmt.Errorf("failed to save instance and cleanup runtime: save error: %w, remove error: %v", err, removeErr) + } return nil, err } return instanceWithID, nil } +// Start starts a runtime instance by ID. +func (m *manager) Start(ctx context.Context, id string) error { + m.mu.Lock() + defer m.mu.Unlock() + + instances, err := m.loadInstances() + if err != nil { + return err + } + + // Find the instance + var instanceToStart Instance + var index int + found := false + for i, instance := range instances { + if instance.GetID() == id { + instanceToStart = instance + index = i + found = true + break + } + } + + if !found { + return ErrInstanceNotFound + } + + runtimeData := instanceToStart.GetRuntimeData() + if runtimeData.Type == "" || runtimeData.InstanceID == "" { + return errors.New("instance has no runtime configured") + } + + // Get the runtime + rt, err := m.runtimeRegistry.Get(runtimeData.Type) + if err != nil { + return fmt.Errorf("failed to get runtime: %w", err) + } + + // Start the runtime instance + runtimeInfo, err := rt.Start(ctx, runtimeData.InstanceID) + if err != nil { + return fmt.Errorf("failed to start runtime instance: %w", err) + } + + // Update the instance with new runtime state + updatedInstance := &instance{ + ID: instanceToStart.GetID(), + Name: instanceToStart.GetName(), + SourceDir: instanceToStart.GetSourceDir(), + ConfigDir: instanceToStart.GetConfigDir(), + Runtime: RuntimeData{ + Type: runtimeData.Type, + InstanceID: runtimeData.InstanceID, + State: runtimeInfo.State, + Info: runtimeInfo.Info, + }, + } + + instances[index] = updatedInstance + return m.saveInstances(instances) +} + +// Stop stops a runtime instance by ID. +func (m *manager) Stop(ctx context.Context, id string) error { + m.mu.Lock() + defer m.mu.Unlock() + + instances, err := m.loadInstances() + if err != nil { + return err + } + + // Find the instance + var instanceToStop Instance + var index int + found := false + for i, instance := range instances { + if instance.GetID() == id { + instanceToStop = instance + index = i + found = true + break + } + } + + if !found { + return ErrInstanceNotFound + } + + runtimeData := instanceToStop.GetRuntimeData() + if runtimeData.Type == "" || runtimeData.InstanceID == "" { + return errors.New("instance has no runtime configured") + } + + // Get the runtime + rt, err := m.runtimeRegistry.Get(runtimeData.Type) + if err != nil { + return fmt.Errorf("failed to get runtime: %w", err) + } + + // Stop the runtime instance + err = rt.Stop(ctx, runtimeData.InstanceID) + if err != nil { + return fmt.Errorf("failed to stop runtime instance: %w", err) + } + + // Get updated runtime info + runtimeInfo, err := rt.Info(ctx, runtimeData.InstanceID) + if err != nil { + return fmt.Errorf("failed to get runtime info: %w", err) + } + + // Update the instance with new runtime state + updatedInstance := &instance{ + ID: instanceToStop.GetID(), + Name: instanceToStop.GetName(), + SourceDir: instanceToStop.GetSourceDir(), + ConfigDir: instanceToStop.GetConfigDir(), + Runtime: RuntimeData{ + Type: runtimeData.Type, + InstanceID: runtimeData.InstanceID, + State: runtimeInfo.State, + Info: runtimeInfo.Info, + }, + } + + instances[index] = updatedInstance + return m.saveInstances(instances) +} + // List returns all registered instances func (m *manager) List() ([]Instance, error) { m.mu.RLock() @@ -178,8 +352,10 @@ func (m *manager) Get(id string) (Instance, error) { return nil, ErrInstanceNotFound } -// Delete unregisters an instance by ID -func (m *manager) Delete(id string) error { +// Delete unregisters an instance by ID. +// Before removing from storage, it attempts to remove the runtime instance. +// Runtime cleanup is best-effort: if the runtime is unavailable, deletion proceeds anyway. +func (m *manager) Delete(ctx context.Context, id string) error { m.mu.Lock() defer m.mu.Unlock() @@ -188,13 +364,15 @@ func (m *manager) Delete(id string) error { return err } - // Find and remove by ID + // Find the instance to delete + var instanceToDelete Instance found := false filtered := make([]Instance, 0, len(instances)) for _, instance := range instances { if instance.GetID() != id { filtered = append(filtered, instance) } else { + instanceToDelete = instance found = true } } @@ -203,7 +381,33 @@ func (m *manager) Delete(id string) error { return ErrInstanceNotFound } - return m.saveInstances(filtered) + // Runtime cleanup: capture errors to avoid orphaning runtime instances + var removeErr error + runtimeInfo := instanceToDelete.GetRuntimeData() + if runtimeInfo.Type != "" && runtimeInfo.InstanceID != "" { + rt, err := m.runtimeRegistry.Get(runtimeInfo.Type) + if err == nil { + // Runtime is available, try to clean up + removeErr = rt.Remove(ctx, runtimeInfo.InstanceID) + } + // If runtime is not available, proceed with deletion anyway (best effort) + } + + // Save the updated instances list (remove from manager) + saveErr := m.saveInstances(filtered) + if saveErr != nil { + if removeErr != nil { + return fmt.Errorf("failed to remove runtime and save instances: remove error: %v, save error: %w", removeErr, saveErr) + } + return saveErr + } + + // If save succeeded but remove failed, we have an orphaned runtime + if removeErr != nil { + return fmt.Errorf("instance removed from manager but runtime cleanup failed: %w", removeErr) + } + + return nil } // Reconcile removes instances with inaccessible directories @@ -237,6 +441,11 @@ func (m *manager) Reconcile() ([]string, error) { return removed, nil } +// RegisterRuntime registers a runtime with the manager's registry. +func (m *manager) RegisterRuntime(rt runtime.Runtime) error { + return m.runtimeRegistry.Register(rt) +} + // generateUniqueName generates a unique name from the source directory // by extracting the last component of the path and adding an increment if needed func (m *manager) generateUniqueName(sourceDir string, instances []Instance) string { diff --git a/pkg/instances/manager_test.go b/pkg/instances/manager_test.go index 8bdb6e0..341cb88 100644 --- a/pkg/instances/manager_test.go +++ b/pkg/instances/manager_test.go @@ -15,6 +15,7 @@ package instances import ( + "context" "encoding/json" "errors" "fmt" @@ -22,6 +23,9 @@ import ( "path/filepath" "sync" "testing" + + "github.com/kortex-hub/kortex-cli/pkg/runtime" + "github.com/kortex-hub/kortex-cli/pkg/runtime/fake" ) // fakeInstance is a test double for the Instance interface @@ -31,6 +35,7 @@ type fakeInstance struct { sourceDir string configDir string accessible bool + runtime RuntimeData } // Compile-time check to ensure fakeInstance implements Instance interface @@ -56,6 +61,14 @@ func (f *fakeInstance) IsAccessible() bool { return f.accessible } +func (f *fakeInstance) GetRuntimeType() string { + return f.runtime.Type +} + +func (f *fakeInstance) GetRuntimeData() RuntimeData { + return f.runtime +} + func (f *fakeInstance) Dump() InstanceData { return InstanceData{ ID: f.id, @@ -64,6 +77,7 @@ func (f *fakeInstance) Dump() InstanceData { Source: f.sourceDir, Configuration: f.configDir, }, + Runtime: f.runtime, } } @@ -74,6 +88,7 @@ type newFakeInstanceParams struct { SourceDir string ConfigDir string Accessible bool + Runtime RuntimeData } // newFakeInstance creates a new fake instance for testing @@ -84,6 +99,7 @@ func newFakeInstance(params newFakeInstanceParams) Instance { sourceDir: params.SourceDir, configDir: params.ConfigDir, accessible: params.Accessible, + runtime: params.Runtime, } } @@ -109,6 +125,7 @@ func fakeInstanceFactory(data InstanceData) (Instance, error) { sourceDir: data.Paths.Source, configDir: data.Paths.Configuration, accessible: true, + runtime: data.Runtime, }, nil } @@ -153,6 +170,13 @@ func (g *fakeSequentialGenerator) Generate() string { return id } +// newTestRegistry creates a runtime registry with a fake runtime for testing +func newTestRegistry() runtime.Registry { + reg := runtime.NewRegistry() + _ = reg.Register(fake.New()) + return reg +} + func TestNewManager(t *testing.T) { t.Parallel() @@ -211,9 +235,9 @@ func TestNewManager(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, err := NewManager(tmpDir) + manager, err := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) if err != nil { - t.Fatalf("NewManager() unexpected error = %v", err) + t.Fatalf("newManagerWithFactory() unexpected error = %v", err) } // We can't directly access storageFile since it's on the unexported struct, @@ -224,7 +248,10 @@ func TestNewManager(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "config"), Accessible: true, }) - _, _ = manager.Add(inst) + _, addErr := manager.Add(context.Background(), inst, "fake") + if addErr != nil { + t.Fatalf("Failed to add instance: %v", addErr) + } expectedFile := filepath.Join(tmpDir, DefaultStorageFileName) if _, err := os.Stat(expectedFile); os.IsNotExist(err) { @@ -240,7 +267,7 @@ func TestManager_Add(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() inst := newFakeInstance(newFakeInstanceParams{ @@ -248,7 +275,7 @@ func TestManager_Add(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "config"), Accessible: true, }) - added, err := manager.Add(inst) + added, err := manager.Add(context.Background(), inst, "fake") if err != nil { t.Fatalf("Add() unexpected error = %v", err) } @@ -270,9 +297,9 @@ func TestManager_Add(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) - _, err := manager.Add(nil) + _, err := manager.Add(context.Background(), nil, "fake") if err == nil { t.Error("Add() expected error for nil instance, got nil") } @@ -281,6 +308,7 @@ func TestManager_Add(t *testing.T) { t.Run("generates unique IDs when adding instances", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() // Create a sequential generator that returns duplicate ID first, then unique ones // Sequence: "duplicate-id", "duplicate-id", "unique-id-1" @@ -291,7 +319,7 @@ func TestManager_Add(t *testing.T) { "duplicate-id-0000000000000000000000000000000000000000000000000000000a", "unique-id-1-0000000000000000000000000000000000000000000000000000000b", }) - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, gen) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, gen, newTestRegistry()) instanceTmpDir := t.TempDir() // Create instances without IDs (empty ID) @@ -306,8 +334,8 @@ func TestManager_Add(t *testing.T) { Accessible: true, }) - added1, _ := manager.Add(inst1) - added2, _ := manager.Add(inst2) + added1, _ := manager.Add(ctx, inst1, "fake") + added2, _ := manager.Add(ctx, inst2, "fake") id1 := added1.GetID() id2 := added2.GetID() @@ -342,7 +370,7 @@ func TestManager_Add(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() inst := newFakeInstance(newFakeInstanceParams{ @@ -350,7 +378,7 @@ func TestManager_Add(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "config"), Accessible: true, }) - _, _ = manager.Add(inst) + _, _ = manager.Add(context.Background(), inst, "fake") // Check that JSON file exists and is readable storageFile := filepath.Join(tmpDir, DefaultStorageFileName) @@ -366,8 +394,9 @@ func TestManager_Add(t *testing.T) { t.Run("can add multiple instances", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() inst1 := newFakeInstance(newFakeInstanceParams{ @@ -386,9 +415,9 @@ func TestManager_Add(t *testing.T) { Accessible: true, }) - _, _ = manager.Add(inst1) - _, _ = manager.Add(inst2) - _, _ = manager.Add(inst3) + _, _ = manager.Add(ctx, inst1, "fake") + _, _ = manager.Add(ctx, inst2, "fake") + _, _ = manager.Add(ctx, inst3, "fake") instances, _ := manager.List() if len(instances) != 3 { @@ -404,7 +433,7 @@ func TestManager_List(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instances, err := manager.List() if err != nil { @@ -418,8 +447,9 @@ func TestManager_List(t *testing.T) { t.Run("returns all added instances", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() inst1 := newFakeInstance(newFakeInstanceParams{ @@ -433,8 +463,8 @@ func TestManager_List(t *testing.T) { Accessible: true, }) - _, _ = manager.Add(inst1) - _, _ = manager.Add(inst2) + _, _ = manager.Add(ctx, inst1, "fake") + _, _ = manager.Add(ctx, inst2, "fake") instances, err := manager.List() if err != nil { @@ -449,7 +479,7 @@ func TestManager_List(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) // Create empty storage file storageFile := filepath.Join(tmpDir, DefaultStorageFileName) @@ -472,7 +502,7 @@ func TestManager_Get(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() expectedSource := filepath.Join(instanceTmpDir, "source") @@ -482,7 +512,7 @@ func TestManager_Get(t *testing.T) { ConfigDir: expectedConfig, Accessible: true, }) - added, _ := manager.Add(inst) + added, _ := manager.Add(context.Background(), inst, "fake") generatedID := added.GetID() @@ -505,7 +535,7 @@ func TestManager_Get(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) _, err := manager.Get("nonexistent-id") if err != ErrInstanceNotFound { @@ -520,8 +550,9 @@ func TestManager_Delete(t *testing.T) { t.Run("deletes existing instance successfully", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() sourceDir := filepath.Join(instanceTmpDir, "source") @@ -531,11 +562,11 @@ func TestManager_Delete(t *testing.T) { ConfigDir: configDir, Accessible: true, }) - added, _ := manager.Add(inst) + added, _ := manager.Add(ctx, inst, "fake") generatedID := added.GetID() - err := manager.Delete(generatedID) + err := manager.Delete(ctx, generatedID) if err != nil { t.Fatalf("Delete() unexpected error = %v", err) } @@ -551,9 +582,9 @@ func TestManager_Delete(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) - err := manager.Delete("nonexistent-id") + err := manager.Delete(context.Background(), "nonexistent-id") if err != ErrInstanceNotFound { t.Errorf("Delete() error = %v, want %v", err, ErrInstanceNotFound) } @@ -562,8 +593,9 @@ func TestManager_Delete(t *testing.T) { t.Run("deletes only specified instance", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() source1 := filepath.Join(instanceTmpDir, "source1") @@ -580,13 +612,13 @@ func TestManager_Delete(t *testing.T) { ConfigDir: config2, Accessible: true, }) - added1, _ := manager.Add(inst1) - added2, _ := manager.Add(inst2) + added1, _ := manager.Add(ctx, inst1, "fake") + added2, _ := manager.Add(ctx, inst2, "fake") id1 := added1.GetID() id2 := added2.GetID() - manager.Delete(id1) + manager.Delete(ctx, id1) // Verify inst2 still exists _, err := manager.Get(id2) @@ -604,22 +636,23 @@ func TestManager_Delete(t *testing.T) { t.Run("verifies deletion is persisted", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() - manager1, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager1, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) inst := newFakeInstance(newFakeInstanceParams{ SourceDir: filepath.Join(string(filepath.Separator), "tmp", "source"), ConfigDir: filepath.Join(string(filepath.Separator), "tmp", "config"), Accessible: true, }) - added, _ := manager1.Add(inst) + added, _ := manager1.Add(ctx, inst, "fake") generatedID := added.GetID() - manager1.Delete(generatedID) + manager1.Delete(ctx, generatedID) // Create new manager with same storage - manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) _, err := manager2.Get(generatedID) if err != ErrInstanceNotFound { t.Errorf("Get() from new manager error = %v, want %v", err, ErrInstanceNotFound) @@ -627,6 +660,274 @@ func TestManager_Delete(t *testing.T) { }) } +func TestManager_Start(t *testing.T) { + t.Parallel() + + t.Run("starts instance successfully", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, _ := manager.Add(ctx, inst, "fake") + + // After Add, state should be "created" + if added.GetRuntimeData().State != "created" { + t.Errorf("After Add, state = %v, want 'created'", added.GetRuntimeData().State) + } + + // Start the instance + err := manager.Start(ctx, added.GetID()) + if err != nil { + t.Fatalf("Start() unexpected error = %v", err) + } + + // Verify state is now "running" + updated, _ := manager.Get(added.GetID()) + if updated.GetRuntimeData().State != "running" { + t.Errorf("After Start, state = %v, want 'running'", updated.GetRuntimeData().State) + } + }) + + t.Run("returns error for nonexistent instance", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + err := manager.Start(context.Background(), "nonexistent-id") + if err != ErrInstanceNotFound { + t.Errorf("Start() error = %v, want %v", err, ErrInstanceNotFound) + } + }) + + t.Run("returns error for instance without runtime", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + // Custom factory that creates instances without runtime + noRuntimeFactory := func(data InstanceData) (Instance, error) { + if data.ID == "" { + return nil, errors.New("instance ID cannot be empty") + } + if data.Name == "" { + return nil, errors.New("instance name cannot be empty") + } + return &fakeInstance{ + id: data.ID, + name: data.Name, + sourceDir: data.Paths.Source, + configDir: data.Paths.Configuration, + accessible: true, + runtime: RuntimeData{}, // Empty runtime + }, nil + } + manager, _ := newManagerWithFactory(tmpDir, noRuntimeFactory, newFakeGenerator(), newTestRegistry()) + + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(string(filepath.Separator), "tmp", "source"), + ConfigDir: filepath.Join(string(filepath.Separator), "tmp", "config"), + Accessible: true, + }) + added, _ := manager.Add(ctx, inst, "fake") + + // Manually save instance without runtime to simulate old data + instances := []Instance{&fakeInstance{ + id: added.GetID(), + name: added.GetName(), + sourceDir: added.GetSourceDir(), + configDir: added.GetConfigDir(), + accessible: true, + runtime: RuntimeData{}, // No runtime + }} + data := make([]InstanceData, len(instances)) + for i, instance := range instances { + data[i] = instance.Dump() + } + jsonData, _ := json.Marshal(data) + storageFile := filepath.Join(tmpDir, DefaultStorageFileName) + os.WriteFile(storageFile, jsonData, 0644) + + err := manager.Start(ctx, added.GetID()) + if err == nil { + t.Error("Start() expected error for instance without runtime, got nil") + } + if err != nil && err.Error() != "instance has no runtime configured" { + t.Errorf("Start() error = %v, want 'instance has no runtime configured'", err) + } + }) + + t.Run("persists state change to storage", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + manager1, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(string(filepath.Separator), "tmp", "source"), + ConfigDir: filepath.Join(string(filepath.Separator), "tmp", "config"), + Accessible: true, + }) + added, _ := manager1.Add(ctx, inst, "fake") + manager1.Start(ctx, added.GetID()) + + // Create new manager with same storage + manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + retrieved, _ := manager2.Get(added.GetID()) + + if retrieved.GetRuntimeData().State != "running" { + t.Errorf("State from new manager = %v, want 'running'", retrieved.GetRuntimeData().State) + } + }) +} + +func TestManager_Stop(t *testing.T) { + t.Parallel() + + t.Run("stops instance successfully", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, _ := manager.Add(ctx, inst, "fake") + manager.Start(ctx, added.GetID()) + + // Verify state is "running" + running, _ := manager.Get(added.GetID()) + if running.GetRuntimeData().State != "running" { + t.Errorf("Before Stop, state = %v, want 'running'", running.GetRuntimeData().State) + } + + // Stop the instance + err := manager.Stop(ctx, added.GetID()) + if err != nil { + t.Fatalf("Stop() unexpected error = %v", err) + } + + // Verify state is now "stopped" + updated, _ := manager.Get(added.GetID()) + if updated.GetRuntimeData().State != "stopped" { + t.Errorf("After Stop, state = %v, want 'stopped'", updated.GetRuntimeData().State) + } + }) + + t.Run("returns error for nonexistent instance", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + err := manager.Stop(context.Background(), "nonexistent-id") + if err != ErrInstanceNotFound { + t.Errorf("Stop() error = %v, want %v", err, ErrInstanceNotFound) + } + }) + + t.Run("returns error for instance without runtime", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(string(filepath.Separator), "tmp", "source"), + ConfigDir: filepath.Join(string(filepath.Separator), "tmp", "config"), + Accessible: true, + }) + added, _ := manager.Add(ctx, inst, "fake") + + // Manually save instance without runtime to simulate old data + instances := []Instance{&fakeInstance{ + id: added.GetID(), + name: added.GetName(), + sourceDir: added.GetSourceDir(), + configDir: added.GetConfigDir(), + accessible: true, + runtime: RuntimeData{}, // No runtime + }} + data := make([]InstanceData, len(instances)) + for i, instance := range instances { + data[i] = instance.Dump() + } + jsonData, _ := json.Marshal(data) + storageFile := filepath.Join(tmpDir, DefaultStorageFileName) + os.WriteFile(storageFile, jsonData, 0644) + + err := manager.Stop(ctx, added.GetID()) + if err == nil { + t.Error("Stop() expected error for instance without runtime, got nil") + } + if err != nil && err.Error() != "instance has no runtime configured" { + t.Errorf("Stop() error = %v, want 'instance has no runtime configured'", err) + } + }) + + t.Run("persists state change to storage", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + manager1, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(string(filepath.Separator), "tmp", "source"), + ConfigDir: filepath.Join(string(filepath.Separator), "tmp", "config"), + Accessible: true, + }) + added, _ := manager1.Add(ctx, inst, "fake") + manager1.Start(ctx, added.GetID()) + manager1.Stop(ctx, added.GetID()) + + // Create new manager with same storage + manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + retrieved, _ := manager2.Get(added.GetID()) + + if retrieved.GetRuntimeData().State != "stopped" { + t.Errorf("State from new manager = %v, want 'stopped'", retrieved.GetRuntimeData().State) + } + }) + + t.Run("can stop created instance that was never started", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + tmpDir := t.TempDir() + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, _ := manager.Add(ctx, inst, "fake") + + // State is "created", try to stop + err := manager.Stop(ctx, added.GetID()) + if err == nil { + t.Error("Stop() expected error for instance in 'created' state, got nil") + } + }) +} + func TestManager_Reconcile(t *testing.T) { t.Parallel() @@ -653,7 +954,7 @@ func TestManager_Reconcile(t *testing.T) { accessible: false, // Always inaccessible for this test }, nil } - manager, _ := newManagerWithFactory(tmpDir, inaccessibleFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, inaccessibleFactory, newFakeGenerator(), newTestRegistry()) // Add instance that is inaccessible instanceTmpDir := t.TempDir() @@ -662,7 +963,7 @@ func TestManager_Reconcile(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "config"), Accessible: false, }) - _, _ = manager.Add(inst) + _, _ = manager.Add(context.Background(), inst, "fake") removed, err := manager.Reconcile() if err != nil { @@ -697,7 +998,7 @@ func TestManager_Reconcile(t *testing.T) { accessible: false, // Always inaccessible for this test }, nil } - manager, _ := newManagerWithFactory(tmpDir, inaccessibleFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, inaccessibleFactory, newFakeGenerator(), newTestRegistry()) // Add instance that is inaccessible instanceTmpDir := t.TempDir() @@ -706,7 +1007,7 @@ func TestManager_Reconcile(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "nonexistent-config"), Accessible: false, }) - _, _ = manager.Add(inst) + _, _ = manager.Add(context.Background(), inst, "fake") removed, err := manager.Reconcile() if err != nil { @@ -741,7 +1042,7 @@ func TestManager_Reconcile(t *testing.T) { accessible: false, // Always inaccessible for this test }, nil } - manager, _ := newManagerWithFactory(tmpDir, inaccessibleFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, inaccessibleFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() inaccessibleSource := filepath.Join(instanceTmpDir, "nonexistent-source") @@ -750,7 +1051,7 @@ func TestManager_Reconcile(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "config"), Accessible: false, }) - added, _ := manager.Add(inst) + added, _ := manager.Add(context.Background(), inst, "fake") generatedID := added.GetID() @@ -767,6 +1068,7 @@ func TestManager_Reconcile(t *testing.T) { t.Run("keeps accessible instances", func(t *testing.T) { t.Parallel() + ctx := context.Background() tmpDir := t.TempDir() instanceTmpDir := t.TempDir() @@ -789,7 +1091,7 @@ func TestManager_Reconcile(t *testing.T) { accessible: accessible, }, nil } - manager, _ := newManagerWithFactory(tmpDir, mixedFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, mixedFactory, newFakeGenerator(), newTestRegistry()) accessibleConfig := filepath.Join(instanceTmpDir, "accessible-config") @@ -799,7 +1101,7 @@ func TestManager_Reconcile(t *testing.T) { ConfigDir: accessibleConfig, Accessible: true, }) - _, _ = manager.Add(accessible) + _, _ = manager.Add(ctx, accessible, "fake") // Add inaccessible instance inaccessible := newFakeInstance(newFakeInstanceParams{ @@ -807,7 +1109,7 @@ func TestManager_Reconcile(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "nonexistent-config"), Accessible: false, }) - _, _ = manager.Add(inaccessible) + _, _ = manager.Add(ctx, inaccessible, "fake") removed, err := manager.Reconcile() if err != nil { @@ -832,7 +1134,7 @@ func TestManager_Reconcile(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() inst := newFakeInstance(newFakeInstanceParams{ @@ -840,7 +1142,7 @@ func TestManager_Reconcile(t *testing.T) { ConfigDir: filepath.Join(instanceTmpDir, "config"), Accessible: true, }) - _, _ = manager.Add(inst) + _, _ = manager.Add(context.Background(), inst, "fake") removed, err := manager.Reconcile() if err != nil { @@ -856,7 +1158,7 @@ func TestManager_Reconcile(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) removed, err := manager.Reconcile() if err != nil { @@ -879,7 +1181,7 @@ func TestManager_Persistence(t *testing.T) { instanceTmpDir := t.TempDir() // Create first manager and add instance - manager1, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager1, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) expectedSource := filepath.Join(instanceTmpDir, "source") expectedConfig := filepath.Join(instanceTmpDir, "config") inst := newFakeInstance(newFakeInstanceParams{ @@ -887,12 +1189,12 @@ func TestManager_Persistence(t *testing.T) { ConfigDir: expectedConfig, Accessible: true, }) - added, _ := manager1.Add(inst) + added, _ := manager1.Add(context.Background(), inst, "fake") generatedID := added.GetID() // Create second manager with same storage - manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instances, err := manager2.List() if err != nil { t.Fatalf("List() from second manager unexpected error = %v", err) @@ -913,7 +1215,7 @@ func TestManager_Persistence(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() expectedSource := filepath.Join(instanceTmpDir, "source") @@ -923,7 +1225,7 @@ func TestManager_Persistence(t *testing.T) { ConfigDir: expectedConfig, Accessible: true, }) - added, _ := manager.Add(inst) + added, _ := manager.Add(context.Background(), inst, "fake") generatedID := added.GetID() @@ -965,7 +1267,7 @@ func TestManager_ConcurrentAccess(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() var wg sync.WaitGroup @@ -982,7 +1284,7 @@ func TestManager_ConcurrentAccess(t *testing.T) { ConfigDir: configDir, Accessible: true, }) - _, _ = manager.Add(inst) + _, _ = manager.Add(context.Background(), inst, "fake") }(i) } @@ -998,7 +1300,7 @@ func TestManager_ConcurrentAccess(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) instanceTmpDir := t.TempDir() // Add some instances first @@ -1010,7 +1312,7 @@ func TestManager_ConcurrentAccess(t *testing.T) { ConfigDir: configDir, Accessible: true, }) - _, _ = manager.Add(inst) + _, _ = manager.Add(context.Background(), inst, "fake") } var wg sync.WaitGroup @@ -1042,7 +1344,7 @@ func TestManager_ensureUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) // Cast to concrete type to access unexported methods mgr := m.(*manager) @@ -1075,7 +1377,7 @@ func TestManager_ensureUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1100,7 +1402,7 @@ func TestManager_ensureUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1139,7 +1441,7 @@ func TestManager_ensureUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1175,7 +1477,7 @@ func TestManager_ensureUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1196,7 +1498,7 @@ func TestManager_generateUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1213,7 +1515,7 @@ func TestManager_generateUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1238,7 +1540,7 @@ func TestManager_generateUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) @@ -1256,7 +1558,7 @@ func TestManager_generateUniqueName(t *testing.T) { t.Parallel() tmpDir := t.TempDir() - m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator()) + m, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), newTestRegistry()) mgr := m.(*manager) From 82889d6672e33c96dca8fa9408ebd5e44cedc00b Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 12 Mar 2026 15:02:35 +0100 Subject: [PATCH 2/5] fix(cmd): register runtime in workspace remove for proper cleanup The workspace remove command was creating a manager without registering any runtimes, causing Delete() to skip backend cleanup with an empty runtime registry. Now registers the fake runtime in preRun() to enable proper cleanup operations. Additionally, made the fake runtime's Remove() method idempotent to handle the non-persistent nature of fake runtime instances (each New() creates separate in-memory state). This prevents errors in tests where instances are created with one runtime and removed with another. Co-Authored-By: Claude Code (Claude Sonnet 4.5) Signed-off-by: Philippe Martin --- pkg/cmd/workspace_remove.go | 8 ++++++++ pkg/runtime/fake/fake.go | 9 ++++++++- pkg/runtime/fake/fake_test.go | 7 +++++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/workspace_remove.go b/pkg/cmd/workspace_remove.go index 0227fff..e0a02c9 100644 --- a/pkg/cmd/workspace_remove.go +++ b/pkg/cmd/workspace_remove.go @@ -26,6 +26,7 @@ import ( api "github.com/kortex-hub/kortex-cli-api/cli/go" "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/kortex-hub/kortex-cli/pkg/runtime/fake" "github.com/spf13/cobra" ) @@ -69,6 +70,13 @@ func (w *workspaceRemoveCmd) preRun(cmd *cobra.Command, args []string) error { if err != nil { return outputErrorIfJSON(cmd, w.output, fmt.Errorf("failed to create manager: %w", err)) } + + // Register fake runtime (for testing) + // TODO: In production, register only the runtimes that are available/configured + if err := manager.RegisterRuntime(fake.New()); err != nil { + return fmt.Errorf("failed to register fake runtime: %w", err) + } + w.manager = manager return nil diff --git a/pkg/runtime/fake/fake.go b/pkg/runtime/fake/fake.go index 0908a18..5207278 100644 --- a/pkg/runtime/fake/fake.go +++ b/pkg/runtime/fake/fake.go @@ -157,7 +157,14 @@ func (f *fakeRuntime) Remove(ctx context.Context, id string) error { inst, exists := f.instances[id] if !exists { - return fmt.Errorf("%w: %s", runtime.ErrInstanceNotFound, id) + // TODO: The fake runtime is not persistent - each New() creates a separate + // in-memory instance that doesn't share state. This causes issues in tests + // where one manager creates instances and another manager (with a new fake + // runtime) tries to remove them. Consider implementing persistent storage + // for the fake runtime (e.g., file-based or shared in-memory registry) to + // better simulate real runtimes (Docker/Podman) which maintain state externally. + // For now, treat missing instances as already removed (idempotent operation). + return nil } if inst.state == "running" { diff --git a/pkg/runtime/fake/fake_test.go b/pkg/runtime/fake/fake_test.go index 9d43f3e..65958ef 100644 --- a/pkg/runtime/fake/fake_test.go +++ b/pkg/runtime/fake/fake_test.go @@ -207,9 +207,12 @@ func TestFakeRuntime_UnknownInstanceID(t *testing.T) { } // Try to remove non-existent instance + // Note: Remove is idempotent for fake runtime (returns nil for non-existent instances) + // because the fake runtime is not persistent and doesn't share state across instances. + // This differs from real runtimes but is appropriate for testing. err = rt.Remove(ctx, "unknown-id") - if !errors.Is(err, runtime.ErrInstanceNotFound) { - t.Errorf("Expected ErrInstanceNotFound, got %v", err) + if err != nil { + t.Errorf("Expected nil (idempotent remove), got %v", err) } // Try to get info for non-existent instance From cc3f3ec4806d530f66a5192de1bd086020856f69 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 12 Mar 2026 15:06:20 +0100 Subject: [PATCH 3/5] fix(instances): add transactional rollback to Start and Stop The Start and Stop methods were calling runtime operations before persisting state, which could leave runtime state and instances.json out of sync if saveInstances() failed. Now both methods use transactional rollback: if persistence fails, the runtime operation is reversed (Stop after Start, Start after Stop) to restore the original state, mirroring the pattern used in Add(). Co-Authored-By: Claude Code (Claude Sonnet 4.5) Signed-off-by: Philippe Martin --- pkg/instances/manager.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/instances/manager.go b/pkg/instances/manager.go index 5f10b3d..c2e445c 100644 --- a/pkg/instances/manager.go +++ b/pkg/instances/manager.go @@ -253,7 +253,18 @@ func (m *manager) Start(ctx context.Context, id string) error { } instances[index] = updatedInstance - return m.saveInstances(instances) + + // Save instances with rollback on failure + if err := m.saveInstances(instances); err != nil { + // Rollback: try to stop the runtime instance to restore original state + stopErr := rt.Stop(ctx, runtimeData.InstanceID) + if stopErr != nil { + return fmt.Errorf("failed to save instance state and rollback runtime: save error: %w, stop error: %v", err, stopErr) + } + return err + } + + return nil } // Stop stops a runtime instance by ID. @@ -321,7 +332,18 @@ func (m *manager) Stop(ctx context.Context, id string) error { } instances[index] = updatedInstance - return m.saveInstances(instances) + + // Save instances with rollback on failure + if err := m.saveInstances(instances); err != nil { + // Rollback: try to start the runtime instance to restore original state + _, startErr := rt.Start(ctx, runtimeData.InstanceID) + if startErr != nil { + return fmt.Errorf("failed to save instance state and rollback runtime: save error: %w, start error: %v", err, startErr) + } + return err + } + + return nil } // List returns all registered instances From 3f90d8c6b6bb26ebd9d17d98ef4c5d2fe2a2df20 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 12 Mar 2026 15:21:09 +0100 Subject: [PATCH 4/5] fix(instances): prevent orphaned runtimes in Delete Delete now requires successful runtime cleanup before removing the instance from storage. Previously, the instance would be deleted even if runtime cleanup failed, leaving orphaned runtime instances. Added comprehensive tests with configurable failable runtime to verify transactional rollback in Start/Stop and proper Delete behavior when runtime operations fail. Co-Authored-By: Claude Code (Claude Sonnet 4.5) Signed-off-by: Philippe Martin --- pkg/instances/manager.go | 29 +- pkg/instances/manager_runtime_test.go | 699 ++++++++++++++++++++++++++ 2 files changed, 710 insertions(+), 18 deletions(-) create mode 100644 pkg/instances/manager_runtime_test.go diff --git a/pkg/instances/manager.go b/pkg/instances/manager.go index c2e445c..b40baa4 100644 --- a/pkg/instances/manager.go +++ b/pkg/instances/manager.go @@ -375,8 +375,8 @@ func (m *manager) Get(id string) (Instance, error) { } // Delete unregisters an instance by ID. -// Before removing from storage, it attempts to remove the runtime instance. -// Runtime cleanup is best-effort: if the runtime is unavailable, deletion proceeds anyway. +// Runtime cleanup is performed first and must succeed before removing from storage. +// If runtime cleanup fails, the instance remains in storage to prevent orphaned runtimes. func (m *manager) Delete(ctx context.Context, id string) error { m.mu.Lock() defer m.mu.Unlock() @@ -403,30 +403,23 @@ func (m *manager) Delete(ctx context.Context, id string) error { return ErrInstanceNotFound } - // Runtime cleanup: capture errors to avoid orphaning runtime instances - var removeErr error + // Runtime cleanup: must succeed before removing from storage runtimeInfo := instanceToDelete.GetRuntimeData() if runtimeInfo.Type != "" && runtimeInfo.InstanceID != "" { rt, err := m.runtimeRegistry.Get(runtimeInfo.Type) - if err == nil { - // Runtime is available, try to clean up - removeErr = rt.Remove(ctx, runtimeInfo.InstanceID) + if err != nil { + return fmt.Errorf("failed to get runtime for cleanup: %w", err) } - // If runtime is not available, proceed with deletion anyway (best effort) - } - // Save the updated instances list (remove from manager) - saveErr := m.saveInstances(filtered) - if saveErr != nil { - if removeErr != nil { - return fmt.Errorf("failed to remove runtime and save instances: remove error: %v, save error: %w", removeErr, saveErr) + // Remove runtime instance first - if this fails, we don't delete from storage + if err := rt.Remove(ctx, runtimeInfo.InstanceID); err != nil { + return fmt.Errorf("failed to remove runtime instance: %w", err) } - return saveErr } - // If save succeeded but remove failed, we have an orphaned runtime - if removeErr != nil { - return fmt.Errorf("instance removed from manager but runtime cleanup failed: %w", removeErr) + // Only save the updated instances list after successful runtime cleanup + if err := m.saveInstances(filtered); err != nil { + return fmt.Errorf("runtime removed but failed to save instances: %w", err) } return nil diff --git a/pkg/instances/manager_runtime_test.go b/pkg/instances/manager_runtime_test.go new file mode 100644 index 0000000..89335df --- /dev/null +++ b/pkg/instances/manager_runtime_test.go @@ -0,0 +1,699 @@ +// Copyright 2026 Red Hat, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package instances + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/kortex-hub/kortex-cli/pkg/runtime" +) + +// failableRuntime is a fake runtime that can be configured to fail on specific operations +type failableRuntime struct { + mu sync.RWMutex + instances map[string]*runtimeInstance + nextID int + failOnCreate bool + failOnStart bool + failOnStop bool + failOnRemove bool + failOnInfo bool + createErr error + startErr error + stopErr error + removeErr error + infoErr error + startCallCount int + stopCallCount int +} + +type runtimeInstance struct { + id string + name string + state string + source string + config string +} + +var _ runtime.Runtime = (*failableRuntime)(nil) + +func newFailableRuntime() *failableRuntime { + return &failableRuntime{ + instances: make(map[string]*runtimeInstance), + nextID: 1, + } +} + +func (f *failableRuntime) Type() string { + return "failable" +} + +func (f *failableRuntime) Create(ctx context.Context, params runtime.CreateParams) (runtime.RuntimeInfo, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if f.failOnCreate { + if f.createErr != nil { + return runtime.RuntimeInfo{}, f.createErr + } + return runtime.RuntimeInfo{}, errors.New("create failed") + } + + id := fmt.Sprintf("failable-%03d", f.nextID) + f.nextID++ + + inst := &runtimeInstance{ + id: id, + name: params.Name, + state: "created", + source: params.SourcePath, + config: params.ConfigPath, + } + + f.instances[id] = inst + + return runtime.RuntimeInfo{ + ID: id, + State: inst.state, + Info: map[string]string{"source": inst.source, "config": inst.config}, + }, nil +} + +func (f *failableRuntime) Start(ctx context.Context, id string) (runtime.RuntimeInfo, error) { + f.mu.Lock() + defer f.mu.Unlock() + + f.startCallCount++ + + if f.failOnStart { + if f.startErr != nil { + return runtime.RuntimeInfo{}, f.startErr + } + return runtime.RuntimeInfo{}, errors.New("start failed") + } + + inst, exists := f.instances[id] + if !exists { + return runtime.RuntimeInfo{}, fmt.Errorf("%w: %s", runtime.ErrInstanceNotFound, id) + } + + inst.state = "running" + + return runtime.RuntimeInfo{ + ID: inst.id, + State: inst.state, + Info: map[string]string{"source": inst.source, "config": inst.config}, + }, nil +} + +func (f *failableRuntime) Stop(ctx context.Context, id string) error { + f.mu.Lock() + defer f.mu.Unlock() + + f.stopCallCount++ + + if f.failOnStop { + if f.stopErr != nil { + return f.stopErr + } + return errors.New("stop failed") + } + + inst, exists := f.instances[id] + if !exists { + return fmt.Errorf("%w: %s", runtime.ErrInstanceNotFound, id) + } + + inst.state = "stopped" + return nil +} + +func (f *failableRuntime) Remove(ctx context.Context, id string) error { + f.mu.Lock() + defer f.mu.Unlock() + + if f.failOnRemove { + if f.removeErr != nil { + return f.removeErr + } + return errors.New("remove failed") + } + + _, exists := f.instances[id] + if !exists { + return fmt.Errorf("%w: %s", runtime.ErrInstanceNotFound, id) + } + + delete(f.instances, id) + return nil +} + +func (f *failableRuntime) Info(ctx context.Context, id string) (runtime.RuntimeInfo, error) { + f.mu.RLock() + defer f.mu.RUnlock() + + if f.failOnInfo { + if f.infoErr != nil { + return runtime.RuntimeInfo{}, f.infoErr + } + return runtime.RuntimeInfo{}, errors.New("info failed") + } + + inst, exists := f.instances[id] + if !exists { + return runtime.RuntimeInfo{}, fmt.Errorf("%w: %s", runtime.ErrInstanceNotFound, id) + } + + return runtime.RuntimeInfo{ + ID: inst.id, + State: inst.state, + Info: map[string]string{"source": inst.source, "config": inst.config}, + }, nil +} + +func (f *failableRuntime) getState(id string) string { + f.mu.RLock() + defer f.mu.RUnlock() + + inst, exists := f.instances[id] + if !exists { + return "" + } + return inst.state +} + +func (f *failableRuntime) instanceExists(id string) bool { + f.mu.RLock() + defer f.mu.RUnlock() + + _, exists := f.instances[id] + return exists +} + +func TestManager_Start_FailureRollback(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + t.Run("rolls back runtime state when saveInstances fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + // Create a registry with the failable runtime + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + // Add an instance + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + runtimeID := added.GetRuntimeData().InstanceID + + // Verify initial state is "created" + if state := fakeRT.getState(runtimeID); state != "created" { + t.Errorf("Initial state = %v, want 'created'", state) + } + + // Make the storage file read-only to force saveInstances to fail + storageFile := filepath.Join(tmpDir, DefaultStorageFileName) + if err := makeReadOnly(storageFile); err != nil { + t.Fatalf("Failed to make storage file read-only: %v", err) + } + defer makeWritable(storageFile) + + // Attempt to start - should fail to save + err = manager.Start(ctx, added.GetID()) + if err == nil { + t.Fatal("Start() expected error when saveInstances fails, got nil") + } + + // Verify runtime state was rolled back to "stopped" + // (Start succeeded, then saveInstances failed, so rollback called Stop) + if state := fakeRT.getState(runtimeID); state != "stopped" { + t.Errorf("After rollback, state = %v, want 'stopped'", state) + } + }) + + t.Run("returns error when runtime Start fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + fakeRT.failOnStart = true + fakeRT.startErr = errors.New("start operation not available") + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + // Attempt to start - should fail immediately + err = manager.Start(ctx, added.GetID()) + if err == nil { + t.Fatal("Start() expected error when runtime Start fails, got nil") + } + if !errors.Is(err, fakeRT.startErr) { + t.Errorf("Start() error should contain runtime error, got: %v", err) + } + + // Verify instance state in storage is still "created" + retrieved, _ := manager.Get(added.GetID()) + if state := retrieved.GetRuntimeData().State; state != "created" { + t.Errorf("Instance state = %v, want 'created'", state) + } + }) + + t.Run("fails when rollback Stop also fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + // Make storage file read-only to force saveInstances to fail + storageFile := filepath.Join(tmpDir, DefaultStorageFileName) + if err := makeReadOnly(storageFile); err != nil { + t.Fatalf("Failed to make storage file read-only: %v", err) + } + defer makeWritable(storageFile) + + // Configure runtime to fail on Stop (rollback will fail) + fakeRT.failOnStop = true + fakeRT.stopErr = errors.New("stop not supported") + + // Attempt to start + err = manager.Start(ctx, added.GetID()) + if err == nil { + t.Fatal("Start() expected error, got nil") + } + + // Error should mention both save failure and rollback failure + errMsg := err.Error() + if !strings.Contains(errMsg, "save") || !strings.Contains(errMsg, "stop") { + t.Errorf("Expected error to mention both save and stop failures, got: %v", err) + } + }) +} + +func TestManager_Stop_FailureRollback(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + t.Run("rolls back runtime state when saveInstances fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + // Start the instance first + if err := manager.Start(ctx, added.GetID()); err != nil { + t.Fatalf("Start() failed: %v", err) + } + + runtimeID := added.GetRuntimeData().InstanceID + + // Verify state is "running" + if state := fakeRT.getState(runtimeID); state != "running" { + t.Errorf("State after Start = %v, want 'running'", state) + } + + // Make the storage file read-only to force saveInstances to fail + storageFile := filepath.Join(tmpDir, DefaultStorageFileName) + if err := makeReadOnly(storageFile); err != nil { + t.Fatalf("Failed to make storage file read-only: %v", err) + } + defer makeWritable(storageFile) + + // Attempt to stop - should fail to save + err = manager.Stop(ctx, added.GetID()) + if err == nil { + t.Fatal("Stop() expected error when saveInstances fails, got nil") + } + + // Verify runtime state was rolled back to "running" + // (Stop succeeded, then saveInstances failed, so rollback called Start) + if state := fakeRT.getState(runtimeID); state != "running" { + t.Errorf("After rollback, state = %v, want 'running'", state) + } + }) + + t.Run("returns error when runtime Stop fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + // Start the instance + if err := manager.Start(ctx, added.GetID()); err != nil { + t.Fatalf("Start() failed: %v", err) + } + + // Configure runtime to fail on Stop + fakeRT.failOnStop = true + fakeRT.stopErr = errors.New("cannot stop running instance") + + // Attempt to stop - should fail immediately + err = manager.Stop(ctx, added.GetID()) + if err == nil { + t.Fatal("Stop() expected error when runtime Stop fails, got nil") + } + if !errors.Is(err, fakeRT.stopErr) { + t.Errorf("Stop() error should contain runtime error, got: %v", err) + } + + // Verify instance state in storage is still "running" + retrieved, _ := manager.Get(added.GetID()) + if state := retrieved.GetRuntimeData().State; state != "running" { + t.Errorf("Instance state = %v, want 'running'", state) + } + }) + + t.Run("fails when rollback Start also fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + // Start the instance + if err := manager.Start(ctx, added.GetID()); err != nil { + t.Fatalf("Start() failed: %v", err) + } + + // Make storage file read-only to force saveInstances to fail + storageFile := filepath.Join(tmpDir, DefaultStorageFileName) + if err := makeReadOnly(storageFile); err != nil { + t.Fatalf("Failed to make storage file read-only: %v", err) + } + defer makeWritable(storageFile) + + // Configure runtime to fail on Start (rollback will fail) + fakeRT.failOnStart = true + fakeRT.startErr = errors.New("start not available") + + // Reset start call count to track rollback attempt + fakeRT.startCallCount = 0 + + // Attempt to stop + err = manager.Stop(ctx, added.GetID()) + if err == nil { + t.Fatal("Stop() expected error, got nil") + } + + // Error should mention both save failure and rollback failure + errMsg := err.Error() + if !strings.Contains(errMsg, "save") || !strings.Contains(errMsg, "start") { + t.Errorf("Expected error to mention both save and start failures, got: %v", err) + } + + // Verify rollback was attempted + if fakeRT.startCallCount == 0 { + t.Error("Expected rollback to attempt Start, but it was not called") + } + }) +} + +func TestManager_Delete_RuntimeCleanupRequired(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + t.Run("does not delete from storage when runtime Remove fails", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + fakeRT.failOnRemove = true + fakeRT.removeErr = errors.New("runtime cannot be removed") + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + instanceID := added.GetID() + runtimeID := added.GetRuntimeData().InstanceID + + // Attempt to delete - should fail due to runtime removal failure + err = manager.Delete(ctx, instanceID) + if err == nil { + t.Fatal("Delete() expected error when runtime Remove fails, got nil") + } + if !errors.Is(err, fakeRT.removeErr) { + t.Errorf("Delete() error should contain runtime error, got: %v", err) + } + + // Verify instance still exists in storage + retrieved, err := manager.Get(instanceID) + if err != nil { + t.Errorf("Get() failed, instance should still exist: %v", err) + } + if retrieved.GetID() != instanceID { + t.Errorf("Retrieved instance ID = %v, want %v", retrieved.GetID(), instanceID) + } + + // Verify runtime instance still exists + if !fakeRT.instanceExists(runtimeID) { + t.Error("Runtime instance should still exist after failed delete") + } + }) + + t.Run("deletes from storage only after successful runtime cleanup", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + instanceID := added.GetID() + runtimeID := added.GetRuntimeData().InstanceID + + // Verify runtime instance exists + if !fakeRT.instanceExists(runtimeID) { + t.Fatal("Runtime instance should exist before delete") + } + + // Delete should succeed + err = manager.Delete(ctx, instanceID) + if err != nil { + t.Fatalf("Delete() failed: %v", err) + } + + // Verify instance was removed from storage + _, err = manager.Get(instanceID) + if err != ErrInstanceNotFound { + t.Errorf("Get() error = %v, want ErrInstanceNotFound", err) + } + + // Verify runtime instance was removed + if fakeRT.instanceExists(runtimeID) { + t.Error("Runtime instance should be removed after successful delete") + } + }) + + t.Run("returns error when runtime not available for cleanup", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + fakeRT := newFailableRuntime() + + reg := runtime.NewRegistry() + if err := reg.Register(fakeRT); err != nil { + t.Fatalf("Failed to register runtime: %v", err) + } + + manager, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), reg) + + instanceTmpDir := t.TempDir() + inst := newFakeInstance(newFakeInstanceParams{ + SourceDir: filepath.Join(instanceTmpDir, "source"), + ConfigDir: filepath.Join(instanceTmpDir, "config"), + Accessible: true, + }) + added, err := manager.Add(ctx, inst, "failable") + if err != nil { + t.Fatalf("Add() failed: %v", err) + } + + instanceID := added.GetID() + + // Create a new manager with empty registry (runtime not available) + manager2, _ := newManagerWithFactory(tmpDir, fakeInstanceFactory, newFakeGenerator(), runtime.NewRegistry()) + + // Attempt to delete - should fail because runtime is not available + err = manager2.Delete(ctx, instanceID) + if err == nil { + t.Fatal("Delete() expected error when runtime not available, got nil") + } + + // Verify instance still exists in storage + retrieved, err := manager2.Get(instanceID) + if err != nil { + t.Errorf("Get() failed, instance should still exist: %v", err) + } + if retrieved.GetID() != instanceID { + t.Errorf("Retrieved instance ID = %v, want %v", retrieved.GetID(), instanceID) + } + }) +} + +// Helper functions + +func chmod(path string, mode uint32) error { + return os.Chmod(path, os.FileMode(mode)) +} + +func makeReadOnly(path string) error { + return chmod(path, 0555) +} + +func makeWritable(path string) error { + return chmod(path, 0755) +} From 10e0f4e47ab2aa8f28b3cb9f5681085464cd5893 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 12 Mar 2026 17:15:33 +0100 Subject: [PATCH 5/5] fix(cmd): use outputErrorIfJSON for runtime registration errors Ensures all errors in preRun methods use the outputErrorIfJSON helper to maintain consistent JSON error output when --output=json is specified. Also updates tests to use the new manager.Add() signature with context and runtime type. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Philippe Martin --- pkg/cmd/init.go | 2 +- pkg/cmd/workspace_remove.go | 2 +- pkg/cmd/workspace_remove_test.go | 17 ++++++++++++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/init.go b/pkg/cmd/init.go index 3a15a9e..416abd2 100644 --- a/pkg/cmd/init.go +++ b/pkg/cmd/init.go @@ -76,7 +76,7 @@ func (i *initCmd) preRun(cmd *cobra.Command, args []string) error { // Register fake runtime (for testing) // TODO: In production, register only the runtimes that are available/configured if err := manager.RegisterRuntime(fake.New()); err != nil { - return fmt.Errorf("failed to register fake runtime: %w", err) + return outputErrorIfJSON(cmd, i.output, fmt.Errorf("failed to register fake runtime: %w", err)) } i.manager = manager diff --git a/pkg/cmd/workspace_remove.go b/pkg/cmd/workspace_remove.go index e0a02c9..6045d27 100644 --- a/pkg/cmd/workspace_remove.go +++ b/pkg/cmd/workspace_remove.go @@ -74,7 +74,7 @@ func (w *workspaceRemoveCmd) preRun(cmd *cobra.Command, args []string) error { // Register fake runtime (for testing) // TODO: In production, register only the runtimes that are available/configured if err := manager.RegisterRuntime(fake.New()); err != nil { - return fmt.Errorf("failed to register fake runtime: %w", err) + return outputErrorIfJSON(cmd, w.output, fmt.Errorf("failed to register fake runtime: %w", err)) } w.manager = manager diff --git a/pkg/cmd/workspace_remove_test.go b/pkg/cmd/workspace_remove_test.go index dfabc18..348b113 100644 --- a/pkg/cmd/workspace_remove_test.go +++ b/pkg/cmd/workspace_remove_test.go @@ -518,7 +518,12 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance: %v", err) } - addedInstance, err := manager.Add(instance) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance, err := manager.Add(context.Background(), instance, "fake") if err != nil { t.Fatalf("Failed to add instance: %v", err) } @@ -620,6 +625,7 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Run("json output removes correct workspace when multiple exist", func(t *testing.T) { t.Parallel() + ctx := context.Background() storageDir := t.TempDir() sourcesDir1 := t.TempDir() sourcesDir2 := t.TempDir() @@ -646,12 +652,17 @@ func TestWorkspaceRemoveCmd_E2E(t *testing.T) { t.Fatalf("Failed to create instance 2: %v", err) } - addedInstance1, err := manager.Add(instance1) + // Register fake runtime + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("Failed to register fake runtime: %v", err) + } + + addedInstance1, err := manager.Add(ctx, instance1, "fake") if err != nil { t.Fatalf("Failed to add instance 1: %v", err) } - addedInstance2, err := manager.Add(instance2) + addedInstance2, err := manager.Add(ctx, instance2, "fake") if err != nil { t.Fatalf("Failed to add instance 2: %v", err) }