diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index dd14743..ee6757d 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -174,7 +174,7 @@ func (i *Instance) SetOptions(opts *Options) { } // Preserve the original nodes to prevent changing instance location - if i.options != nil && i.options.get() != nil && i.options.get().Nodes != nil { + if i.options != nil && i.options.get() != nil { opts.Nodes = i.options.get().Nodes } diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index ac9019e..375c210 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -8,6 +8,7 @@ import ( "llamactl/pkg/instance" "llamactl/pkg/testutil" "testing" + "time" ) func TestNewInstance(t *testing.T) { @@ -515,3 +516,303 @@ func TestCreateOptionsValidation(t *testing.T) { }) } } + +func TestStatusChangeCallback(t *testing.T) { + backendConfig := &config.BackendConfig{ + LlamaCpp: config.BackendSettings{Command: "llama-server"}, + } + globalSettings := &config.InstancesConfig{LogsDir: "/tmp/test"} + options := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + } + + var callbackOldStatus, callbackNewStatus instance.Status + callbackCalled := false + + onStatusChange := func(oldStatus, newStatus instance.Status) { + callbackOldStatus = oldStatus + callbackNewStatus = newStatus + callbackCalled = true + } + + inst := instance.New("test", backendConfig, globalSettings, options, "main", onStatusChange) + + inst.SetStatus(instance.Running) + + if !callbackCalled { + t.Error("Expected status change callback to be called") + } + if callbackOldStatus != instance.Stopped { + t.Errorf("Expected old status Stopped, got %v", callbackOldStatus) + } + if callbackNewStatus != instance.Running { + t.Errorf("Expected new status Running, got %v", callbackNewStatus) + } +} + +func TestSetOptions_NodesPreserved(t *testing.T) { + backendConfig := &config.BackendConfig{ + LlamaCpp: config.BackendSettings{Command: "llama-server"}, + } + globalSettings := &config.InstancesConfig{LogsDir: "/tmp/test"} + + tests := []struct { + name string + initialNodes map[string]struct{} + updateNodes map[string]struct{} + expectedNodes map[string]struct{} + }{ + { + name: "nil nodes preserved as nil", + initialNodes: nil, + updateNodes: map[string]struct{}{"worker1": {}}, + expectedNodes: nil, + }, + { + name: "empty nodes preserved as empty", + initialNodes: map[string]struct{}{}, + updateNodes: map[string]struct{}{"worker1": {}}, + expectedNodes: map[string]struct{}{}, + }, + { + name: "existing nodes preserved", + initialNodes: map[string]struct{}{"worker1": {}, "worker2": {}}, + updateNodes: map[string]struct{}{"worker3": {}}, + expectedNodes: map[string]struct{}{"worker1": {}, "worker2": {}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + options := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + Nodes: tt.initialNodes, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + } + + inst := instance.New("test", backendConfig, globalSettings, options, "main", nil) + + // Attempt to update nodes (should be ignored) + updateOptions := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + Nodes: tt.updateNodes, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/new-model.gguf", + }, + } + inst.SetOptions(updateOptions) + + opts := inst.GetOptions() + + // Verify nodes are preserved + if len(opts.Nodes) != len(tt.expectedNodes) { + t.Errorf("Expected %d nodes, got %d", len(tt.expectedNodes), len(opts.Nodes)) + } + for node := range tt.expectedNodes { + if _, exists := opts.Nodes[node]; !exists { + t.Errorf("Expected node %s to exist", node) + } + } + + // Verify other options were updated + if opts.LlamaServerOptions.Model != "/path/to/new-model.gguf" { + t.Errorf("Expected model to be updated to '/path/to/new-model.gguf', got %q", opts.LlamaServerOptions.Model) + } + }) + } +} + +func TestProcessErrorCases(t *testing.T) { + backendConfig := &config.BackendConfig{ + LlamaCpp: config.BackendSettings{Command: "llama-server"}, + } + globalSettings := &config.InstancesConfig{LogsDir: "/tmp/test"} + options := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + } + + inst := instance.New("test", backendConfig, globalSettings, options, "main", nil) + + // Stop when not running should return error + err := inst.Stop() + if err == nil { + t.Error("Expected error when stopping non-running instance") + } + + // Simulate running state + inst.SetStatus(instance.Running) + + // Start when already running should return error + err = inst.Start() + if err == nil { + t.Error("Expected error when starting already running instance") + } +} + +func TestRemoteInstanceOperations(t *testing.T) { + backendConfig := &config.BackendConfig{ + LlamaCpp: config.BackendSettings{Command: "llama-server"}, + } + globalSettings := &config.InstancesConfig{LogsDir: "/tmp/test"} + options := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + Nodes: map[string]struct{}{"remote-node": {}}, // Remote instance + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + } + + inst := instance.New("remote-test", backendConfig, globalSettings, options, "main", nil) + + if !inst.IsRemote() { + t.Error("Expected instance to be remote") + } + + // Start should fail for remote instance + if err := inst.Start(); err == nil { + t.Error("Expected error when starting remote instance") + } + + // Stop should fail for remote instance + if err := inst.Stop(); err == nil { + t.Error("Expected error when stopping remote instance") + } + + // Restart should fail for remote instance + if err := inst.Restart(); err == nil { + t.Error("Expected error when restarting remote instance") + } + + // GetProxy should fail for remote instance + if _, err := inst.GetProxy(); err == nil { + t.Error("Expected error when getting proxy for remote instance") + } + + // GetLogs should fail for remote instance + if _, err := inst.GetLogs(10); err == nil { + t.Error("Expected error when getting logs for remote instance") + } +} + +func TestProxyClearOnOptionsChange(t *testing.T) { + backendConfig := &config.BackendConfig{ + LlamaCpp: config.BackendSettings{Command: "llama-server"}, + } + globalSettings := &config.InstancesConfig{LogsDir: "/tmp/test"} + options := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Host: "localhost", + Port: 8080, + }, + } + + inst := instance.New("test", backendConfig, globalSettings, options, "main", nil) + + // Get initial proxy + proxy1, err := inst.GetProxy() + if err != nil { + t.Fatalf("Failed to get initial proxy: %v", err) + } + + // Update options (should clear proxy) + newOptions := &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Host: "localhost", + Port: 8081, // Different port + }, + } + inst.SetOptions(newOptions) + + // Get proxy again - should be recreated with new port + proxy2, err := inst.GetProxy() + if err != nil { + t.Fatalf("Failed to get proxy after options change: %v", err) + } + + // Proxies should be different instances (recreated) + if proxy1 == proxy2 { + t.Error("Expected proxy to be recreated after options change") + } +} + +func TestIdleTimeout(t *testing.T) { + backendConfig := &config.BackendConfig{ + LlamaCpp: config.BackendSettings{Command: "llama-server"}, + } + globalSettings := &config.InstancesConfig{LogsDir: "/tmp/test"} + + t.Run("not running never times out", func(t *testing.T) { + timeout := 1 + inst := instance.New("test", backendConfig, globalSettings, &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + IdleTimeout: &timeout, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + }, "main", nil) + + if inst.ShouldTimeout() { + t.Error("Non-running instance should never timeout") + } + }) + + t.Run("no timeout configured", func(t *testing.T) { + inst := instance.New("test", backendConfig, globalSettings, &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + IdleTimeout: nil, // No timeout + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + }, "main", nil) + inst.SetStatus(instance.Running) + + if inst.ShouldTimeout() { + t.Error("Instance with no timeout configured should not timeout") + } + }) + + t.Run("timeout exceeded", func(t *testing.T) { + timeout := 1 // 1 minute + inst := instance.New("test", backendConfig, globalSettings, &instance.Options{ + BackendType: backends.BackendTypeLlamaCpp, + IdleTimeout: &timeout, + LlamaServerOptions: &llamacpp.LlamaServerOptions{ + Model: "/path/to/model.gguf", + }, + }, "main", nil) + inst.SetStatus(instance.Running) + + // Use mock time provider + mockTime := &mockTimeProvider{currentTime: time.Now().Unix()} + inst.SetTimeProvider(mockTime) + + // Set last request time to now + inst.UpdateLastRequestTime() + + // Advance time by 2 minutes (exceeds 1 minute timeout) + mockTime.currentTime = time.Now().Add(2 * time.Minute).Unix() + + if !inst.ShouldTimeout() { + t.Error("Instance should timeout when idle time exceeds configured timeout") + } + }) +} + +// mockTimeProvider for timeout testing +type mockTimeProvider struct { + currentTime int64 // Unix timestamp +} + +func (m *mockTimeProvider) Now() time.Time { + return time.Unix(m.currentTime, 0) +} diff --git a/pkg/instance/timeout_test.go b/pkg/instance/timeout_test.go deleted file mode 100644 index e49e893..0000000 --- a/pkg/instance/timeout_test.go +++ /dev/null @@ -1,274 +0,0 @@ -package instance_test - -import ( - "llamactl/pkg/backends" - "llamactl/pkg/backends/llamacpp" - "llamactl/pkg/config" - "llamactl/pkg/instance" - "llamactl/pkg/testutil" - "sync/atomic" - "testing" - "time" -) - -// MockTimeProvider implements TimeProvider for testing -type MockTimeProvider struct { - currentTime atomic.Int64 // Unix timestamp -} - -func NewMockTimeProvider(t time.Time) *MockTimeProvider { - m := &MockTimeProvider{} - m.currentTime.Store(t.Unix()) - return m -} - -func (m *MockTimeProvider) Now() time.Time { - return time.Unix(m.currentTime.Load(), 0) -} - -func (m *MockTimeProvider) SetTime(t time.Time) { - m.currentTime.Store(t.Unix()) -} - -// Timeout-related tests - -func TestUpdateLastRequestTime(t *testing.T) { - backendConfig := &config.BackendConfig{ - LlamaCpp: config.BackendSettings{ - Command: "llama-server", - }, - MLX: config.BackendSettings{ - Command: "mlx_lm.server", - }, - } - - globalSettings := &config.InstancesConfig{ - LogsDir: "/tmp/test", - } - - options := &instance.Options{ - BackendType: backends.BackendTypeLlamaCpp, - LlamaServerOptions: &llamacpp.LlamaServerOptions{ - Model: "/path/to/model.gguf", - }, - } - - // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - - inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) - - // Test that UpdateLastRequestTime doesn't panic - inst.UpdateLastRequestTime() -} - -func TestShouldTimeout_NotRunning(t *testing.T) { - backendConfig := &config.BackendConfig{ - LlamaCpp: config.BackendSettings{ - Command: "llama-server", - }, - MLX: config.BackendSettings{ - Command: "mlx_lm.server", - }, - } - - globalSettings := &config.InstancesConfig{ - LogsDir: "/tmp/test", - } - - idleTimeout := 1 // 1 minute - options := &instance.Options{ - IdleTimeout: &idleTimeout, - BackendType: backends.BackendTypeLlamaCpp, - LlamaServerOptions: &llamacpp.LlamaServerOptions{ - Model: "/path/to/model.gguf", - }, - } - - // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - - inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) - - // Instance is not running, should not timeout regardless of configuration - if inst.ShouldTimeout() { - t.Error("Non-running instance should never timeout") - } -} - -func TestShouldTimeout_NoTimeoutConfigured(t *testing.T) { - backendConfig := &config.BackendConfig{ - LlamaCpp: config.BackendSettings{ - Command: "llama-server", - }, - MLX: config.BackendSettings{ - Command: "mlx_lm.server", - }, - } - - globalSettings := &config.InstancesConfig{ - LogsDir: "/tmp/test", - } - - tests := []struct { - name string - idleTimeout *int - }{ - {"nil timeout", nil}, - {"zero timeout", testutil.IntPtr(0)}, - {"negative timeout", testutil.IntPtr(-5)}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - - options := &instance.Options{ - IdleTimeout: tt.idleTimeout, - BackendType: backends.BackendTypeLlamaCpp, - LlamaServerOptions: &llamacpp.LlamaServerOptions{ - Model: "/path/to/model.gguf", - }, - } - - inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) - // Simulate running state - inst.SetStatus(instance.Running) - - if inst.ShouldTimeout() { - t.Errorf("Instance with %s should not timeout", tt.name) - } - }) - } -} - -func TestShouldTimeout_WithinTimeLimit(t *testing.T) { - backendConfig := &config.BackendConfig{ - LlamaCpp: config.BackendSettings{ - Command: "llama-server", - }, - MLX: config.BackendSettings{ - Command: "mlx_lm.server", - }, - } - - globalSettings := &config.InstancesConfig{ - LogsDir: "/tmp/test", - } - - idleTimeout := 5 // 5 minutes - options := &instance.Options{ - IdleTimeout: &idleTimeout, - BackendType: backends.BackendTypeLlamaCpp, - LlamaServerOptions: &llamacpp.LlamaServerOptions{ - Model: "/path/to/model.gguf", - }, - } - - // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - - inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) - inst.SetStatus(instance.Running) - - // Update last request time to now - inst.UpdateLastRequestTime() - - // Should not timeout immediately - if inst.ShouldTimeout() { - t.Error("Instance should not timeout when last request was recent") - } -} - -func TestShouldTimeout_ExceedsTimeLimit(t *testing.T) { - backendConfig := &config.BackendConfig{ - LlamaCpp: config.BackendSettings{ - Command: "llama-server", - }, - MLX: config.BackendSettings{ - Command: "mlx_lm.server", - }, - } - - globalSettings := &config.InstancesConfig{ - LogsDir: "/tmp/test", - } - - idleTimeout := 1 // 1 minute - options := &instance.Options{ - IdleTimeout: &idleTimeout, - BackendType: backends.BackendTypeLlamaCpp, - LlamaServerOptions: &llamacpp.LlamaServerOptions{ - Model: "/path/to/model.gguf", - }, - } - - // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - - inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) - inst.SetStatus(instance.Running) - - // Use MockTimeProvider to simulate old last request time - mockTime := NewMockTimeProvider(time.Now()) - inst.SetTimeProvider(mockTime) - - // Set last request time to now - inst.UpdateLastRequestTime() - - // Advance time by 2 minutes (exceeds 1 minute timeout) - mockTime.SetTime(time.Now().Add(2 * time.Minute)) - - if !inst.ShouldTimeout() { - t.Error("Instance should timeout when last request exceeds idle timeout") - } -} - -func TestTimeoutConfiguration_Validation(t *testing.T) { - backendConfig := &config.BackendConfig{ - LlamaCpp: config.BackendSettings{ - Command: "llama-server", - }, - MLX: config.BackendSettings{ - Command: "mlx_lm.server", - }, - } - - globalSettings := &config.InstancesConfig{ - LogsDir: "/tmp/test", - } - - tests := []struct { - name string - inputTimeout *int - expectedTimeout int - }{ - {"default value when nil", nil, 0}, - {"positive value", testutil.IntPtr(10), 10}, - {"zero value", testutil.IntPtr(0), 0}, - {"negative value gets corrected", testutil.IntPtr(-5), 0}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - options := &instance.Options{ - IdleTimeout: tt.inputTimeout, - BackendType: backends.BackendTypeLlamaCpp, - LlamaServerOptions: &llamacpp.LlamaServerOptions{ - Model: "/path/to/model.gguf", - }, - } - - // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - - inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) - opts := inst.GetOptions() - - if opts.IdleTimeout == nil || *opts.IdleTimeout != tt.expectedTimeout { - t.Errorf("Expected IdleTimeout %d, got %v", tt.expectedTimeout, opts.IdleTimeout) - } - }) - } -}