From ef3478e2a30f6d54c1efba218759ba0de7869e81 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Tue, 14 Oct 2025 19:32:15 +0200 Subject: [PATCH 01/16] Move logging to separate struct --- pkg/instance/instance.go | 4 ++++ pkg/instance/{logging.go => logger.go} | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) rename pkg/instance/{logging.go => logger.go} (90%) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index dcebef4..e555cae 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -311,3 +311,7 @@ func (i *Process) IsRemote() bool { return len(i.options.Nodes) > 0 } + +func (i *Process) GetLogs(num_lines int) (string, error) { + return i.logger.GetLogs(num_lines) +} diff --git a/pkg/instance/logging.go b/pkg/instance/logger.go similarity index 90% rename from pkg/instance/logging.go rename to pkg/instance/logger.go index 5432556..5c993df 100644 --- a/pkg/instance/logging.go +++ b/pkg/instance/logger.go @@ -6,6 +6,7 @@ import ( "io" "os" "strings" + "sync" "time" ) @@ -14,6 +15,7 @@ type InstanceLogger struct { logDir string logFile *os.File logFilePath string + mu sync.RWMutex } func NewInstanceLogger(name string, logDir string) *InstanceLogger { @@ -25,6 +27,9 @@ func NewInstanceLogger(name string, logDir string) *InstanceLogger { // Create creates and opens the log files for stdout and stderr func (i *InstanceLogger) Create() error { + i.mu.Lock() + defer i.mu.Unlock() + if i.logDir == "" { return fmt.Errorf("logDir is empty for instance %s", i.name) } @@ -52,16 +57,15 @@ func (i *InstanceLogger) Create() error { } // GetLogs retrieves the last n lines of logs from the instance -func (i *Process) GetLogs(num_lines int) (string, error) { +func (i *InstanceLogger) GetLogs(num_lines int) (string, error) { i.mu.RLock() - logFileName := i.logger.logFilePath - i.mu.RUnlock() + defer i.mu.RUnlock() - if logFileName == "" { - return "", fmt.Errorf("log file not created for instance %s", i.Name) + if i.logFilePath == "" { + return "", fmt.Errorf("log file not created for instance %s", i.name) } - file, err := os.Open(logFileName) + file, err := os.Open(i.logFilePath) if err != nil { return "", fmt.Errorf("failed to open log file: %w", err) } @@ -95,6 +99,9 @@ func (i *Process) GetLogs(num_lines int) (string, error) { // closeLogFile closes the log files func (i *InstanceLogger) Close() { + i.mu.Lock() + defer i.mu.Unlock() + if i.logFile != nil { timestamp := time.Now().Format("2006-01-02 15:04:05") fmt.Fprintf(i.logFile, "=== Instance %s stopped at %s ===\n\n", i.name, timestamp) From 02909c51535141f2045f5240a5a7393ea7290356 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Tue, 14 Oct 2025 19:46:43 +0200 Subject: [PATCH 02/16] Remove redundant instance prefix from logger --- pkg/instance/instance.go | 2 +- pkg/instance/logger.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index e555cae..6ab771b 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -44,7 +44,7 @@ type Process struct { Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created // Logging file - logger *InstanceLogger `json:"-"` + logger *Logger `json:"-"` // internal cmd *exec.Cmd `json:"-"` // Command to run the instance diff --git a/pkg/instance/logger.go b/pkg/instance/logger.go index 5c993df..10db0fb 100644 --- a/pkg/instance/logger.go +++ b/pkg/instance/logger.go @@ -10,7 +10,7 @@ import ( "time" ) -type InstanceLogger struct { +type Logger struct { name string logDir string logFile *os.File @@ -18,15 +18,15 @@ type InstanceLogger struct { mu sync.RWMutex } -func NewInstanceLogger(name string, logDir string) *InstanceLogger { - return &InstanceLogger{ +func NewInstanceLogger(name string, logDir string) *Logger { + return &Logger{ name: name, logDir: logDir, } } // Create creates and opens the log files for stdout and stderr -func (i *InstanceLogger) Create() error { +func (i *Logger) Create() error { i.mu.Lock() defer i.mu.Unlock() @@ -57,7 +57,7 @@ func (i *InstanceLogger) Create() error { } // GetLogs retrieves the last n lines of logs from the instance -func (i *InstanceLogger) GetLogs(num_lines int) (string, error) { +func (i *Logger) GetLogs(num_lines int) (string, error) { i.mu.RLock() defer i.mu.RUnlock() @@ -98,7 +98,7 @@ func (i *InstanceLogger) GetLogs(num_lines int) (string, error) { } // closeLogFile closes the log files -func (i *InstanceLogger) Close() { +func (i *Logger) Close() { i.mu.Lock() defer i.mu.Unlock() @@ -111,7 +111,7 @@ func (i *InstanceLogger) Close() { } // readOutput reads from the given reader and writes lines to the log file -func (i *InstanceLogger) readOutput(reader io.ReadCloser) { +func (i *Logger) readOutput(reader io.ReadCloser) { defer reader.Close() scanner := bufio.NewScanner(reader) From 92a76bc84b3d628e7b4923217beb5c2ae40f3b3f Mon Sep 17 00:00:00 2001 From: LordMathis Date: Tue, 14 Oct 2025 22:01:09 +0200 Subject: [PATCH 03/16] Move proxy to separate struct --- pkg/instance/instance.go | 119 ++++++++------------------- pkg/instance/lifecycle.go | 14 ++-- pkg/instance/proxy.go | 165 ++++++++++++++++++++++++++++++++++++++ pkg/instance/timeout.go | 26 ++---- 4 files changed, 214 insertions(+), 110 deletions(-) create mode 100644 pkg/instance/proxy.go diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 6ab771b..63774f1 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -8,12 +8,9 @@ import ( "llamactl/pkg/backends" "llamactl/pkg/config" "log" - "net/http" "net/http/httputil" - "net/url" "os/exec" "sync" - "sync/atomic" "time" ) @@ -46,23 +43,24 @@ type Process struct { // Logging file logger *Logger `json:"-"` + // Proxy component + proxy *Proxy `json:"-"` // HTTP proxy and request tracking + // internal - cmd *exec.Cmd `json:"-"` // Command to run the instance - ctx context.Context `json:"-"` // Context for managing the instance lifecycle - cancel context.CancelFunc `json:"-"` // Function to cancel the context - stdout io.ReadCloser `json:"-"` // Standard output stream - stderr io.ReadCloser `json:"-"` // Standard error stream - mu sync.RWMutex `json:"-"` // RWMutex for better read/write separation - restarts int `json:"-"` // Number of restarts - proxy *httputil.ReverseProxy `json:"-"` // Reverse proxy for this instance + cmd *exec.Cmd `json:"-"` // Command to run the instance + ctx context.Context `json:"-"` // Context for managing the instance lifecycle + cancel context.CancelFunc `json:"-"` // Function to cancel the context + stdout io.ReadCloser `json:"-"` // Standard output stream + stderr io.ReadCloser `json:"-"` // Standard error stream + mu sync.RWMutex `json:"-"` // RWMutex for better read/write separation + restarts int `json:"-"` // Number of restarts // Restart control restartCancel context.CancelFunc `json:"-"` // Cancel function for pending restarts monitorDone chan struct{} `json:"-"` // Channel to signal monitor goroutine completion - // Timeout management - lastRequestTime atomic.Int64 // Unix timestamp of last request - timeProvider TimeProvider `json:"-"` // Time provider for testing + // Time provider for testing (kept for backward compatibility during refactor) + timeProvider TimeProvider `json:"-"` // Time provider for testing } // NewInstance creates a new instance with the given name, log path, and options @@ -73,7 +71,7 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa // Create the instance logger logger := NewInstanceLogger(name, globalInstanceSettings.LogsDir) - return &Process{ + instance := &Process{ Name: name, options: options, globalInstanceSettings: globalInstanceSettings, @@ -84,6 +82,11 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa Status: Stopped, onStatusChange: onStatusChange, } + + // Create Proxy component + instance.proxy = NewProxy(instance) + + return instance } func (i *Process) GetOptions() *CreateInstanceOptions { @@ -149,88 +152,27 @@ func (i *Process) SetOptions(options *CreateInstanceOptions) { options.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) i.options = options + // Clear the proxy so it gets recreated with new options - i.proxy = nil + if i.proxy != nil { + i.proxy.clearProxy() + } } // SetTimeProvider sets a custom time provider for testing func (i *Process) SetTimeProvider(tp TimeProvider) { i.timeProvider = tp + if i.proxy != nil { + i.proxy.SetTimeProvider(tp) + } } -// GetProxy returns the reverse proxy for this instance, creating it if needed +// GetProxy returns the reverse proxy for this instance, delegating to Proxy component func (i *Process) GetProxy() (*httputil.ReverseProxy, error) { - i.mu.Lock() - defer i.mu.Unlock() - - if i.proxy != nil { - return i.proxy, nil + if i.proxy == nil { + return nil, fmt.Errorf("instance %s has no proxy component", i.Name) } - - if i.options == nil { - return nil, fmt.Errorf("instance %s has no options set", i.Name) - } - - // Remote instances should not use local proxy - they are handled by RemoteInstanceProxy - if len(i.options.Nodes) > 0 { - return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", i.Name) - } - - var host string - var port int - switch i.options.BackendType { - case backends.BackendTypeLlamaCpp: - if i.options.LlamaServerOptions != nil { - host = i.options.LlamaServerOptions.Host - port = i.options.LlamaServerOptions.Port - } - case backends.BackendTypeMlxLm: - if i.options.MlxServerOptions != nil { - host = i.options.MlxServerOptions.Host - port = i.options.MlxServerOptions.Port - } - case backends.BackendTypeVllm: - if i.options.VllmServerOptions != nil { - host = i.options.VllmServerOptions.Host - port = i.options.VllmServerOptions.Port - } - } - - targetURL, err := url.Parse(fmt.Sprintf("http://%s:%d", host, port)) - if err != nil { - return nil, fmt.Errorf("failed to parse target URL for instance %s: %w", i.Name, err) - } - - proxy := httputil.NewSingleHostReverseProxy(targetURL) - - var responseHeaders map[string]string - switch i.options.BackendType { - case backends.BackendTypeLlamaCpp: - responseHeaders = i.globalBackendSettings.LlamaCpp.ResponseHeaders - case backends.BackendTypeVllm: - responseHeaders = i.globalBackendSettings.VLLM.ResponseHeaders - case backends.BackendTypeMlxLm: - responseHeaders = i.globalBackendSettings.MLX.ResponseHeaders - } - proxy.ModifyResponse = func(resp *http.Response) error { - // Remove CORS headers from llama-server response to avoid conflicts - // llamactl will add its own CORS headers - resp.Header.Del("Access-Control-Allow-Origin") - resp.Header.Del("Access-Control-Allow-Methods") - resp.Header.Del("Access-Control-Allow-Headers") - resp.Header.Del("Access-Control-Allow-Credentials") - resp.Header.Del("Access-Control-Max-Age") - resp.Header.Del("Access-Control-Expose-Headers") - - for key, value := range responseHeaders { - resp.Header.Set(key, value) - } - return nil - } - - i.proxy = proxy - - return i.proxy, nil + return i.proxy.GetProxy() } // MarshalJSON implements json.Marshaler for Instance @@ -297,6 +239,9 @@ func (i *Process) UnmarshalJSON(data []byte) error { if i.logger == nil && i.globalInstanceSettings != nil { i.logger = NewInstanceLogger(i.Name, i.globalInstanceSettings.LogsDir) } + if i.proxy == nil { + i.proxy = NewProxy(i) + } return nil } diff --git a/pkg/instance/lifecycle.go b/pkg/instance/lifecycle.go index fa37dc3..dfdeab3 100644 --- a/pkg/instance/lifecycle.go +++ b/pkg/instance/lifecycle.go @@ -36,7 +36,9 @@ func (i *Process) Start() error { } // Initialize last request time to current time when starting - i.lastRequestTime.Store(i.timeProvider.Now().Unix()) + if i.proxy != nil { + i.proxy.UpdateLastRequestTime() + } // Create context before building command (needed for CommandContext) i.ctx, i.cancel = context.WithCancel(context.Background()) @@ -111,9 +113,6 @@ func (i *Process) Stop() error { // Set status to stopped first to signal intentional stop i.SetStatus(Stopped) - // Clean up the proxy - i.proxy = nil - // Get the monitor done channel before releasing the lock monitorDone := i.monitorDone @@ -159,8 +158,13 @@ func (i *Process) Stop() error { return nil } +// LastRequestTime returns the last request time as a Unix timestamp +// Delegates to the Proxy component func (i *Process) LastRequestTime() int64 { - return i.lastRequestTime.Load() + if i.proxy == nil { + return 0 + } + return i.proxy.LastRequestTime() } func (i *Process) WaitForHealthy(timeout int) error { diff --git a/pkg/instance/proxy.go b/pkg/instance/proxy.go new file mode 100644 index 0000000..5727edb --- /dev/null +++ b/pkg/instance/proxy.go @@ -0,0 +1,165 @@ +package instance + +import ( + "fmt" + "llamactl/pkg/backends" + "net/http" + "net/http/httputil" + "net/url" + "sync" + "sync/atomic" +) + +// Proxy manages HTTP reverse proxy and request tracking for an instance. +type Proxy struct { + process *Process // Owner reference - Proxy is owned by Process + + mu sync.RWMutex + proxy *httputil.ReverseProxy + proxyOnce sync.Once + proxyErr error + lastRequestTime atomic.Int64 + timeProvider TimeProvider +} + +// NewProxy creates a new Proxy for the given process +func NewProxy(process *Process) *Proxy { + return &Proxy{ + process: process, + timeProvider: realTimeProvider{}, + } +} + +// GetProxy returns the reverse proxy for this instance, creating it if needed. +// Uses sync.Once to ensure thread-safe one-time initialization. +func (p *Proxy) GetProxy() (*httputil.ReverseProxy, error) { + // sync.Once guarantees buildProxy() is called exactly once + // Other callers block until first initialization completes + p.proxyOnce.Do(func() { + p.proxy, p.proxyErr = p.buildProxy() + }) + + return p.proxy, p.proxyErr +} + +// buildProxy creates the reverse proxy based on instance options +func (p *Proxy) buildProxy() (*httputil.ReverseProxy, error) { + options := p.process.GetOptions() + if options == nil { + return nil, fmt.Errorf("instance %s has no options set", p.process.Name) + } + + // Remote instances should not use local proxy - they are handled by RemoteInstanceProxy + if len(options.Nodes) > 0 { + return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", p.process.Name) + } + + // Get host/port from options + var host string + var port int + switch options.BackendType { + case backends.BackendTypeLlamaCpp: + if options.LlamaServerOptions != nil { + host = options.LlamaServerOptions.Host + port = options.LlamaServerOptions.Port + } + case backends.BackendTypeMlxLm: + if options.MlxServerOptions != nil { + host = options.MlxServerOptions.Host + port = options.MlxServerOptions.Port + } + case backends.BackendTypeVllm: + if options.VllmServerOptions != nil { + host = options.VllmServerOptions.Host + port = options.VllmServerOptions.Port + } + } + + if host == "" { + host = "localhost" + } + + targetURL, err := url.Parse(fmt.Sprintf("http://%s:%d", host, port)) + if err != nil { + return nil, fmt.Errorf("failed to parse target URL for instance %s: %w", p.process.Name, err) + } + + proxy := httputil.NewSingleHostReverseProxy(targetURL) + + // Get response headers from backend config + var responseHeaders map[string]string + switch options.BackendType { + case backends.BackendTypeLlamaCpp: + responseHeaders = p.process.globalBackendSettings.LlamaCpp.ResponseHeaders + case backends.BackendTypeVllm: + responseHeaders = p.process.globalBackendSettings.VLLM.ResponseHeaders + case backends.BackendTypeMlxLm: + responseHeaders = p.process.globalBackendSettings.MLX.ResponseHeaders + } + + proxy.ModifyResponse = func(resp *http.Response) error { + // Remove CORS headers from backend response to avoid conflicts + // llamactl will add its own CORS headers + resp.Header.Del("Access-Control-Allow-Origin") + resp.Header.Del("Access-Control-Allow-Methods") + resp.Header.Del("Access-Control-Allow-Headers") + resp.Header.Del("Access-Control-Allow-Credentials") + resp.Header.Del("Access-Control-Max-Age") + resp.Header.Del("Access-Control-Expose-Headers") + + for key, value := range responseHeaders { + resp.Header.Set(key, value) + } + return nil + } + + return proxy, nil +} + +// clearProxy resets the proxy, allowing it to be recreated when options change. +// This resets the sync.Once so the next GetProxy call will rebuild the proxy. +func (p *Proxy) clearProxy() { + p.mu.Lock() + defer p.mu.Unlock() + + p.proxy = nil + p.proxyErr = nil + p.proxyOnce = sync.Once{} // Reset Once for next GetProxy call +} + +// UpdateLastRequestTime updates the last request access time for the instance +func (p *Proxy) UpdateLastRequestTime() { + lastRequestTime := p.timeProvider.Now().Unix() + p.lastRequestTime.Store(lastRequestTime) +} + +// LastRequestTime returns the last request time as a Unix timestamp +func (p *Proxy) LastRequestTime() int64 { + return p.lastRequestTime.Load() +} + +// ShouldTimeout checks if the instance should timeout based on idle time +func (p *Proxy) ShouldTimeout() bool { + if !p.process.IsRunning() { + return false + } + + options := p.process.GetOptions() + if options == nil || options.IdleTimeout == nil || *options.IdleTimeout <= 0 { + return false + } + + // Check if the last request time exceeds the idle timeout + lastRequest := p.lastRequestTime.Load() + idleTimeoutMinutes := *options.IdleTimeout + + // Convert timeout from minutes to seconds for comparison + idleTimeoutSeconds := int64(idleTimeoutMinutes * 60) + + return (p.timeProvider.Now().Unix() - lastRequest) > idleTimeoutSeconds +} + +// SetTimeProvider sets a custom time provider for testing +func (p *Proxy) SetTimeProvider(tp TimeProvider) { + p.timeProvider = tp +} diff --git a/pkg/instance/timeout.go b/pkg/instance/timeout.go index 94cdc16..acfdc58 100644 --- a/pkg/instance/timeout.go +++ b/pkg/instance/timeout.go @@ -1,28 +1,18 @@ package instance // UpdateLastRequestTime updates the last request access time for the instance via proxy +// Delegates to the Proxy component func (i *Process) UpdateLastRequestTime() { - i.mu.Lock() - defer i.mu.Unlock() - - lastRequestTime := i.timeProvider.Now().Unix() - i.lastRequestTime.Store(lastRequestTime) + if i.proxy != nil { + i.proxy.UpdateLastRequestTime() + } } +// ShouldTimeout checks if the instance should timeout based on idle time +// Delegates to the Proxy component func (i *Process) ShouldTimeout() bool { - i.mu.RLock() - defer i.mu.RUnlock() - - if !i.IsRunning() || i.options.IdleTimeout == nil || *i.options.IdleTimeout <= 0 { + if i.proxy == nil { return false } - - // Check if the last request time exceeds the idle timeout - lastRequest := i.lastRequestTime.Load() - idleTimeoutMinutes := *i.options.IdleTimeout - - // Convert timeout from minutes to seconds for comparison - idleTimeoutSeconds := int64(idleTimeoutMinutes * 60) - - return (i.timeProvider.Now().Unix() - lastRequest) > idleTimeoutSeconds + return i.proxy.ShouldTimeout() } From 964c6345efdd9df0362fd2be0c58f690a1da943b Mon Sep 17 00:00:00 2001 From: LordMathis Date: Tue, 14 Oct 2025 22:16:26 +0200 Subject: [PATCH 04/16] Refactor backend host/port retrieval and remove redundant code for health checks --- pkg/instance/instance.go | 58 +++++++++++++++++++++++++-------------- pkg/instance/lifecycle.go | 31 ++------------------- pkg/instance/proxy.go | 39 ++++++++++---------------- 3 files changed, 55 insertions(+), 73 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 63774f1..e6f9843 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -14,18 +14,6 @@ import ( "time" ) -// TimeProvider interface allows for testing with mock time -type TimeProvider interface { - Now() time.Time -} - -// realTimeProvider implements TimeProvider using the actual time -type realTimeProvider struct{} - -func (realTimeProvider) Now() time.Time { - return time.Now() -} - // Process represents a running instance of the llama server type Process struct { Name string `json:"name"` @@ -58,9 +46,6 @@ type Process struct { // Restart control restartCancel context.CancelFunc `json:"-"` // Cancel function for pending restarts monitorDone chan struct{} `json:"-"` // Channel to signal monitor goroutine completion - - // Time provider for testing (kept for backward compatibility during refactor) - timeProvider TimeProvider `json:"-"` // Time provider for testing } // NewInstance creates a new instance with the given name, log path, and options @@ -77,7 +62,6 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa globalInstanceSettings: globalInstanceSettings, globalBackendSettings: globalBackendSettings, logger: logger, - timeProvider: realTimeProvider{}, Created: time.Now().Unix(), Status: Stopped, onStatusChange: onStatusChange, @@ -160,8 +144,8 @@ func (i *Process) SetOptions(options *CreateInstanceOptions) { } // SetTimeProvider sets a custom time provider for testing +// Delegates to the Proxy component func (i *Process) SetTimeProvider(tp TimeProvider) { - i.timeProvider = tp if i.proxy != nil { i.proxy.SetTimeProvider(tp) } @@ -233,9 +217,6 @@ func (i *Process) UnmarshalJSON(data []byte) error { } // Initialize fields that are not serialized - if i.timeProvider == nil { - i.timeProvider = realTimeProvider{} - } if i.logger == nil && i.globalInstanceSettings != nil { i.logger = NewInstanceLogger(i.Name, i.globalInstanceSettings.LogsDir) } @@ -260,3 +241,40 @@ func (i *Process) IsRemote() bool { func (i *Process) GetLogs(num_lines int) (string, error) { return i.logger.GetLogs(num_lines) } + +// getBackendHostPort extracts the host and port from instance options +// Returns the configured host and port for the backend +func (i *Process) getBackendHostPort() (string, int) { + i.mu.RLock() + defer i.mu.RUnlock() + + if i.options == nil { + return "localhost", 0 + } + + var host string + var port int + switch i.options.BackendType { + case backends.BackendTypeLlamaCpp: + if i.options.LlamaServerOptions != nil { + host = i.options.LlamaServerOptions.Host + port = i.options.LlamaServerOptions.Port + } + case backends.BackendTypeMlxLm: + if i.options.MlxServerOptions != nil { + host = i.options.MlxServerOptions.Host + port = i.options.MlxServerOptions.Port + } + case backends.BackendTypeVllm: + if i.options.VllmServerOptions != nil { + host = i.options.VllmServerOptions.Host + port = i.options.VllmServerOptions.Port + } + } + + if host == "" { + host = "localhost" + } + + return host, port +} diff --git a/pkg/instance/lifecycle.go b/pkg/instance/lifecycle.go index dfdeab3..d530900 100644 --- a/pkg/instance/lifecycle.go +++ b/pkg/instance/lifecycle.go @@ -179,35 +179,8 @@ func (i *Process) WaitForHealthy(timeout int) error { ctx, cancel := context.WithTimeout(context.Background(), time.Duration(timeout)*time.Second) defer cancel() - // Get instance options to build the health check URL - opts := i.GetOptions() - if opts == nil { - return fmt.Errorf("instance %s has no options set", i.Name) - } - - // Build the health check URL directly - var host string - var port int - switch opts.BackendType { - case backends.BackendTypeLlamaCpp: - if opts.LlamaServerOptions != nil { - host = opts.LlamaServerOptions.Host - port = opts.LlamaServerOptions.Port - } - case backends.BackendTypeMlxLm: - if opts.MlxServerOptions != nil { - host = opts.MlxServerOptions.Host - port = opts.MlxServerOptions.Port - } - case backends.BackendTypeVllm: - if opts.VllmServerOptions != nil { - host = opts.VllmServerOptions.Host - port = opts.VllmServerOptions.Port - } - } - if host == "" { - host = "localhost" - } + // Get host/port from process + host, port := i.getBackendHostPort() healthURL := fmt.Sprintf("http://%s:%d/health", host, port) // Create a dedicated HTTP client for health checks diff --git a/pkg/instance/proxy.go b/pkg/instance/proxy.go index 5727edb..d8a7462 100644 --- a/pkg/instance/proxy.go +++ b/pkg/instance/proxy.go @@ -8,8 +8,21 @@ import ( "net/url" "sync" "sync/atomic" + "time" ) +// TimeProvider interface allows for testing with mock time +type TimeProvider interface { + Now() time.Time +} + +// realTimeProvider implements TimeProvider using the actual time +type realTimeProvider struct{} + +func (realTimeProvider) Now() time.Time { + return time.Now() +} + // Proxy manages HTTP reverse proxy and request tracking for an instance. type Proxy struct { process *Process // Owner reference - Proxy is owned by Process @@ -54,30 +67,8 @@ func (p *Proxy) buildProxy() (*httputil.ReverseProxy, error) { return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", p.process.Name) } - // Get host/port from options - var host string - var port int - switch options.BackendType { - case backends.BackendTypeLlamaCpp: - if options.LlamaServerOptions != nil { - host = options.LlamaServerOptions.Host - port = options.LlamaServerOptions.Port - } - case backends.BackendTypeMlxLm: - if options.MlxServerOptions != nil { - host = options.MlxServerOptions.Host - port = options.MlxServerOptions.Port - } - case backends.BackendTypeVllm: - if options.VllmServerOptions != nil { - host = options.VllmServerOptions.Host - port = options.VllmServerOptions.Port - } - } - - if host == "" { - host = "localhost" - } + // Get host/port from process + host, port := p.process.getBackendHostPort() targetURL, err := url.Parse(fmt.Sprintf("http://%s:%d", host, port)) if err != nil { From 80ca0cbd4f9ff53df5c4d41c037c32f450ab343b Mon Sep 17 00:00:00 2001 From: LordMathis Date: Thu, 16 Oct 2025 19:38:44 +0200 Subject: [PATCH 05/16] Rename Process to Instance --- pkg/instance/instance.go | 42 +++++++++++++------------- pkg/instance/instance_test.go | 2 +- pkg/instance/lifecycle.go | 18 +++++------ pkg/instance/logger.go | 14 ++++----- pkg/instance/proxy.go | 24 +++++++-------- pkg/instance/status.go | 6 ++-- pkg/instance/timeout.go | 4 +-- pkg/manager/manager.go | 52 ++++++++++++++++---------------- pkg/manager/operations.go | 18 +++++------ pkg/manager/remote_ops.go | 28 ++++++++--------- pkg/manager/timeout.go | 2 +- pkg/manager/timeout_test.go | 6 ++-- pkg/server/handlers_instances.go | 2 +- pkg/server/handlers_openai.go | 2 +- 14 files changed, 110 insertions(+), 110 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index e6f9843..605fd59 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -14,8 +14,8 @@ import ( "time" ) -// Process represents a running instance of the llama server -type Process struct { +// Instance represents a running instance of the llama server +type Instance struct { Name string `json:"name"` options *CreateInstanceOptions `json:"-"` globalInstanceSettings *config.InstancesConfig @@ -29,10 +29,10 @@ type Process struct { Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created // Logging file - logger *Logger `json:"-"` + logger *logger `json:"-"` // Proxy component - proxy *Proxy `json:"-"` // HTTP proxy and request tracking + proxy *proxy `json:"-"` // HTTP proxy and request tracking // internal cmd *exec.Cmd `json:"-"` // Command to run the instance @@ -49,14 +49,14 @@ type Process struct { } // NewInstance creates a new instance with the given name, log path, and options -func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, options *CreateInstanceOptions, onStatusChange func(oldStatus, newStatus InstanceStatus)) *Process { +func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, options *CreateInstanceOptions, onStatusChange func(oldStatus, newStatus InstanceStatus)) *Instance { // Validate and copy options options.ValidateAndApplyDefaults(name, globalInstanceSettings) // Create the instance logger - logger := NewInstanceLogger(name, globalInstanceSettings.LogsDir) + logger := NewLogger(name, globalInstanceSettings.LogsDir) - instance := &Process{ + instance := &Instance{ Name: name, options: options, globalInstanceSettings: globalInstanceSettings, @@ -73,13 +73,13 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa return instance } -func (i *Process) GetOptions() *CreateInstanceOptions { +func (i *Instance) GetOptions() *CreateInstanceOptions { i.mu.RLock() defer i.mu.RUnlock() return i.options } -func (i *Process) GetPort() int { +func (i *Instance) GetPort() int { i.mu.RLock() defer i.mu.RUnlock() if i.options != nil { @@ -101,7 +101,7 @@ func (i *Process) GetPort() int { return 0 } -func (i *Process) GetHost() string { +func (i *Instance) GetHost() string { i.mu.RLock() defer i.mu.RUnlock() if i.options != nil { @@ -123,7 +123,7 @@ func (i *Process) GetHost() string { return "" } -func (i *Process) SetOptions(options *CreateInstanceOptions) { +func (i *Instance) SetOptions(options *CreateInstanceOptions) { i.mu.Lock() defer i.mu.Unlock() @@ -145,14 +145,14 @@ func (i *Process) SetOptions(options *CreateInstanceOptions) { // SetTimeProvider sets a custom time provider for testing // Delegates to the Proxy component -func (i *Process) SetTimeProvider(tp TimeProvider) { +func (i *Instance) SetTimeProvider(tp TimeProvider) { if i.proxy != nil { i.proxy.SetTimeProvider(tp) } } // GetProxy returns the reverse proxy for this instance, delegating to Proxy component -func (i *Process) GetProxy() (*httputil.ReverseProxy, error) { +func (i *Instance) GetProxy() (*httputil.ReverseProxy, error) { if i.proxy == nil { return nil, fmt.Errorf("instance %s has no proxy component", i.Name) } @@ -160,7 +160,7 @@ func (i *Process) GetProxy() (*httputil.ReverseProxy, error) { } // MarshalJSON implements json.Marshaler for Instance -func (i *Process) MarshalJSON() ([]byte, error) { +func (i *Instance) MarshalJSON() ([]byte, error) { // Use read lock since we're only reading data i.mu.RLock() defer i.mu.RUnlock() @@ -183,7 +183,7 @@ func (i *Process) MarshalJSON() ([]byte, error) { } // Use anonymous struct to avoid recursion - type Alias Process + type Alias Instance return json.Marshal(&struct { *Alias Options *CreateInstanceOptions `json:"options,omitempty"` @@ -196,9 +196,9 @@ func (i *Process) MarshalJSON() ([]byte, error) { } // UnmarshalJSON implements json.Unmarshaler for Instance -func (i *Process) UnmarshalJSON(data []byte) error { +func (i *Instance) UnmarshalJSON(data []byte) error { // Use anonymous struct to avoid recursion - type Alias Process + type Alias Instance aux := &struct { *Alias Options *CreateInstanceOptions `json:"options,omitempty"` @@ -218,7 +218,7 @@ func (i *Process) UnmarshalJSON(data []byte) error { // Initialize fields that are not serialized if i.logger == nil && i.globalInstanceSettings != nil { - i.logger = NewInstanceLogger(i.Name, i.globalInstanceSettings.LogsDir) + i.logger = NewLogger(i.Name, i.globalInstanceSettings.LogsDir) } if i.proxy == nil { i.proxy = NewProxy(i) @@ -227,7 +227,7 @@ func (i *Process) UnmarshalJSON(data []byte) error { return nil } -func (i *Process) IsRemote() bool { +func (i *Instance) IsRemote() bool { i.mu.RLock() defer i.mu.RUnlock() @@ -238,13 +238,13 @@ func (i *Process) IsRemote() bool { return len(i.options.Nodes) > 0 } -func (i *Process) GetLogs(num_lines int) (string, error) { +func (i *Instance) GetLogs(num_lines int) (string, error) { return i.logger.GetLogs(num_lines) } // getBackendHostPort extracts the host and port from instance options // Returns the configured host and port for the backend -func (i *Process) getBackendHostPort() (string, int) { +func (i *Instance) getBackendHostPort() (string, int) { i.mu.RLock() defer i.mu.RUnlock() diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index fc41a94..f22ee72 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -345,7 +345,7 @@ func TestUnmarshalJSON(t *testing.T) { } }` - var inst instance.Process + var inst instance.Instance err := json.Unmarshal([]byte(jsonData), &inst) if err != nil { t.Fatalf("JSON unmarshal failed: %v", err) diff --git a/pkg/instance/lifecycle.go b/pkg/instance/lifecycle.go index d530900..23bd385 100644 --- a/pkg/instance/lifecycle.go +++ b/pkg/instance/lifecycle.go @@ -16,7 +16,7 @@ import ( ) // Start starts the llama server instance and returns an error if it fails. -func (i *Process) Start() error { +func (i *Instance) Start() error { i.mu.Lock() defer i.mu.Unlock() @@ -90,7 +90,7 @@ func (i *Process) Start() error { } // Stop terminates the subprocess -func (i *Process) Stop() error { +func (i *Instance) Stop() error { i.mu.Lock() if !i.IsRunning() { @@ -160,14 +160,14 @@ func (i *Process) Stop() error { // LastRequestTime returns the last request time as a Unix timestamp // Delegates to the Proxy component -func (i *Process) LastRequestTime() int64 { +func (i *Instance) LastRequestTime() int64 { if i.proxy == nil { return 0 } return i.proxy.LastRequestTime() } -func (i *Process) WaitForHealthy(timeout int) error { +func (i *Instance) WaitForHealthy(timeout int) error { if !i.IsRunning() { return fmt.Errorf("instance %s is not running", i.Name) } @@ -226,7 +226,7 @@ func (i *Process) WaitForHealthy(timeout int) error { } } -func (i *Process) monitorProcess() { +func (i *Instance) monitorProcess() { defer func() { i.mu.Lock() if i.monitorDone != nil { @@ -267,7 +267,7 @@ func (i *Process) monitorProcess() { } // handleRestart manages the restart process while holding the lock -func (i *Process) handleRestart() { +func (i *Instance) handleRestart() { // Validate restart conditions and get safe parameters shouldRestart, maxRestarts, restartDelay := i.validateRestartConditions() if !shouldRestart { @@ -310,7 +310,7 @@ func (i *Process) handleRestart() { } // validateRestartConditions checks if the instance should be restarted and returns the parameters -func (i *Process) validateRestartConditions() (shouldRestart bool, maxRestarts int, restartDelay int) { +func (i *Instance) validateRestartConditions() (shouldRestart bool, maxRestarts int, restartDelay int) { if i.options == nil { log.Printf("Instance %s not restarting: options are nil", i.Name) return false, 0, 0 @@ -344,7 +344,7 @@ func (i *Process) validateRestartConditions() (shouldRestart bool, maxRestarts i } // buildCommand builds the command to execute using backend-specific logic -func (i *Process) buildCommand() (*exec.Cmd, error) { +func (i *Instance) buildCommand() (*exec.Cmd, error) { // Get backend configuration backendConfig, err := i.getBackendConfig() if err != nil { @@ -375,7 +375,7 @@ func (i *Process) buildCommand() (*exec.Cmd, error) { } // getBackendConfig resolves the backend configuration for the current instance -func (i *Process) getBackendConfig() (*config.BackendSettings, error) { +func (i *Instance) getBackendConfig() (*config.BackendSettings, error) { var backendTypeStr string switch i.options.BackendType { diff --git a/pkg/instance/logger.go b/pkg/instance/logger.go index 10db0fb..014ca33 100644 --- a/pkg/instance/logger.go +++ b/pkg/instance/logger.go @@ -10,7 +10,7 @@ import ( "time" ) -type Logger struct { +type logger struct { name string logDir string logFile *os.File @@ -18,15 +18,15 @@ type Logger struct { mu sync.RWMutex } -func NewInstanceLogger(name string, logDir string) *Logger { - return &Logger{ +func NewLogger(name string, logDir string) *logger { + return &logger{ name: name, logDir: logDir, } } // Create creates and opens the log files for stdout and stderr -func (i *Logger) Create() error { +func (i *logger) Create() error { i.mu.Lock() defer i.mu.Unlock() @@ -57,7 +57,7 @@ func (i *Logger) Create() error { } // GetLogs retrieves the last n lines of logs from the instance -func (i *Logger) GetLogs(num_lines int) (string, error) { +func (i *logger) GetLogs(num_lines int) (string, error) { i.mu.RLock() defer i.mu.RUnlock() @@ -98,7 +98,7 @@ func (i *Logger) GetLogs(num_lines int) (string, error) { } // closeLogFile closes the log files -func (i *Logger) Close() { +func (i *logger) Close() { i.mu.Lock() defer i.mu.Unlock() @@ -111,7 +111,7 @@ func (i *Logger) Close() { } // readOutput reads from the given reader and writes lines to the log file -func (i *Logger) readOutput(reader io.ReadCloser) { +func (i *logger) readOutput(reader io.ReadCloser) { defer reader.Close() scanner := bufio.NewScanner(reader) diff --git a/pkg/instance/proxy.go b/pkg/instance/proxy.go index d8a7462..1a515bd 100644 --- a/pkg/instance/proxy.go +++ b/pkg/instance/proxy.go @@ -23,9 +23,9 @@ func (realTimeProvider) Now() time.Time { return time.Now() } -// Proxy manages HTTP reverse proxy and request tracking for an instance. -type Proxy struct { - process *Process // Owner reference - Proxy is owned by Process +// proxy manages HTTP reverse proxy and request tracking for an instance. +type proxy struct { + process *Instance // Owner reference - Proxy is owned by Process mu sync.RWMutex proxy *httputil.ReverseProxy @@ -36,8 +36,8 @@ type Proxy struct { } // NewProxy creates a new Proxy for the given process -func NewProxy(process *Process) *Proxy { - return &Proxy{ +func NewProxy(process *Instance) *proxy { + return &proxy{ process: process, timeProvider: realTimeProvider{}, } @@ -45,7 +45,7 @@ func NewProxy(process *Process) *Proxy { // GetProxy returns the reverse proxy for this instance, creating it if needed. // Uses sync.Once to ensure thread-safe one-time initialization. -func (p *Proxy) GetProxy() (*httputil.ReverseProxy, error) { +func (p *proxy) GetProxy() (*httputil.ReverseProxy, error) { // sync.Once guarantees buildProxy() is called exactly once // Other callers block until first initialization completes p.proxyOnce.Do(func() { @@ -56,7 +56,7 @@ func (p *Proxy) GetProxy() (*httputil.ReverseProxy, error) { } // buildProxy creates the reverse proxy based on instance options -func (p *Proxy) buildProxy() (*httputil.ReverseProxy, error) { +func (p *proxy) buildProxy() (*httputil.ReverseProxy, error) { options := p.process.GetOptions() if options == nil { return nil, fmt.Errorf("instance %s has no options set", p.process.Name) @@ -109,7 +109,7 @@ func (p *Proxy) buildProxy() (*httputil.ReverseProxy, error) { // clearProxy resets the proxy, allowing it to be recreated when options change. // This resets the sync.Once so the next GetProxy call will rebuild the proxy. -func (p *Proxy) clearProxy() { +func (p *proxy) clearProxy() { p.mu.Lock() defer p.mu.Unlock() @@ -119,18 +119,18 @@ func (p *Proxy) clearProxy() { } // UpdateLastRequestTime updates the last request access time for the instance -func (p *Proxy) UpdateLastRequestTime() { +func (p *proxy) UpdateLastRequestTime() { lastRequestTime := p.timeProvider.Now().Unix() p.lastRequestTime.Store(lastRequestTime) } // LastRequestTime returns the last request time as a Unix timestamp -func (p *Proxy) LastRequestTime() int64 { +func (p *proxy) LastRequestTime() int64 { return p.lastRequestTime.Load() } // ShouldTimeout checks if the instance should timeout based on idle time -func (p *Proxy) ShouldTimeout() bool { +func (p *proxy) ShouldTimeout() bool { if !p.process.IsRunning() { return false } @@ -151,6 +151,6 @@ func (p *Proxy) ShouldTimeout() bool { } // SetTimeProvider sets a custom time provider for testing -func (p *Proxy) SetTimeProvider(tp TimeProvider) { +func (p *proxy) SetTimeProvider(tp TimeProvider) { p.timeProvider = tp } diff --git a/pkg/instance/status.go b/pkg/instance/status.go index e07fe03..b06bef0 100644 --- a/pkg/instance/status.go +++ b/pkg/instance/status.go @@ -26,7 +26,7 @@ var statusToName = map[InstanceStatus]string{ Failed: "failed", } -func (p *Process) SetStatus(status InstanceStatus) { +func (p *Instance) SetStatus(status InstanceStatus) { oldStatus := p.Status p.Status = status @@ -35,12 +35,12 @@ func (p *Process) SetStatus(status InstanceStatus) { } } -func (p *Process) GetStatus() InstanceStatus { +func (p *Instance) GetStatus() InstanceStatus { return p.Status } // IsRunning returns true if the status is Running -func (p *Process) IsRunning() bool { +func (p *Instance) IsRunning() bool { return p.Status == Running } diff --git a/pkg/instance/timeout.go b/pkg/instance/timeout.go index acfdc58..93e2477 100644 --- a/pkg/instance/timeout.go +++ b/pkg/instance/timeout.go @@ -2,7 +2,7 @@ package instance // UpdateLastRequestTime updates the last request access time for the instance via proxy // Delegates to the Proxy component -func (i *Process) UpdateLastRequestTime() { +func (i *Instance) UpdateLastRequestTime() { if i.proxy != nil { i.proxy.UpdateLastRequestTime() } @@ -10,7 +10,7 @@ func (i *Process) UpdateLastRequestTime() { // ShouldTimeout checks if the instance should timeout based on idle time // Delegates to the Proxy component -func (i *Process) ShouldTimeout() bool { +func (i *Instance) ShouldTimeout() bool { if i.proxy == nil { return false } diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index b944ef3..258d4b7 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -16,35 +16,35 @@ import ( // InstanceManager defines the interface for managing instances of the llama server. type InstanceManager interface { - ListInstances() ([]*instance.Process, error) - CreateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Process, error) - GetInstance(name string) (*instance.Process, error) - UpdateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Process, error) + ListInstances() ([]*instance.Instance, error) + CreateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) + GetInstance(name string) (*instance.Instance, error) + UpdateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) DeleteInstance(name string) error - StartInstance(name string) (*instance.Process, error) + StartInstance(name string) (*instance.Instance, error) IsMaxRunningInstancesReached() bool - StopInstance(name string) (*instance.Process, error) + StopInstance(name string) (*instance.Instance, error) EvictLRUInstance() error - RestartInstance(name string) (*instance.Process, error) + RestartInstance(name string) (*instance.Instance, error) GetInstanceLogs(name string, numLines int) (string, error) Shutdown() } type RemoteManager interface { - ListRemoteInstances(node *config.NodeConfig) ([]*instance.Process, error) - CreateRemoteInstance(node *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Process, error) - GetRemoteInstance(node *config.NodeConfig, name string) (*instance.Process, error) - UpdateRemoteInstance(node *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Process, error) + ListRemoteInstances(node *config.NodeConfig) ([]*instance.Instance, error) + CreateRemoteInstance(node *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) + GetRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) + UpdateRemoteInstance(node *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) DeleteRemoteInstance(node *config.NodeConfig, name string) error - StartRemoteInstance(node *config.NodeConfig, name string) (*instance.Process, error) - StopRemoteInstance(node *config.NodeConfig, name string) (*instance.Process, error) - RestartRemoteInstance(node *config.NodeConfig, name string) (*instance.Process, error) + StartRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) + StopRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) + RestartRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) GetRemoteInstanceLogs(node *config.NodeConfig, name string, numLines int) (string, error) } type instanceManager struct { mu sync.RWMutex - instances map[string]*instance.Process + instances map[string]*instance.Instance runningInstances map[string]struct{} ports map[int]bool instancesConfig config.InstancesConfig @@ -57,9 +57,9 @@ type instanceManager struct { isShutdown bool // Remote instance management - httpClient *http.Client - instanceNodeMap map[string]*config.NodeConfig // Maps instance name to its node config - nodeConfigMap map[string]*config.NodeConfig // Maps node name to node config for quick lookup + httpClient *http.Client + instanceNodeMap map[string]*config.NodeConfig // Maps instance name to its node config + nodeConfigMap map[string]*config.NodeConfig // Maps node name to node config for quick lookup } // NewInstanceManager creates a new instance of InstanceManager. @@ -76,7 +76,7 @@ func NewInstanceManager(backendsConfig config.BackendConfig, instancesConfig con } im := &instanceManager{ - instances: make(map[string]*instance.Process), + instances: make(map[string]*instance.Instance), runningInstances: make(map[string]struct{}), ports: make(map[int]bool), instancesConfig: instancesConfig, @@ -130,7 +130,7 @@ func (im *instanceManager) getNextAvailablePort() (int, error) { } // persistInstance saves an instance to its JSON file -func (im *instanceManager) persistInstance(instance *instance.Process) error { +func (im *instanceManager) persistInstance(instance *instance.Instance) error { if im.instancesConfig.InstancesDir == "" { return nil // Persistence disabled } @@ -172,7 +172,7 @@ func (im *instanceManager) Shutdown() { close(im.shutdownChan) // Create a list of running instances to stop - var runningInstances []*instance.Process + var runningInstances []*instance.Instance var runningNames []string for name, inst := range im.instances { if inst.IsRunning() { @@ -197,7 +197,7 @@ func (im *instanceManager) Shutdown() { wg.Add(len(runningInstances)) for i, inst := range runningInstances { - go func(name string, inst *instance.Process) { + go func(name string, inst *instance.Instance) { defer wg.Done() fmt.Printf("Stopping instance %s...\n", name) // Attempt to stop the instance gracefully @@ -261,7 +261,7 @@ func (im *instanceManager) loadInstance(name, path string) error { return fmt.Errorf("failed to read instance file: %w", err) } - var persistedInstance instance.Process + var persistedInstance instance.Instance if err := json.Unmarshal(data, &persistedInstance); err != nil { return fmt.Errorf("failed to unmarshal instance: %w", err) } @@ -318,8 +318,8 @@ func (im *instanceManager) loadInstance(name, path string) error { // For instances with auto-restart disabled, it sets their status to Stopped func (im *instanceManager) autoStartInstances() { im.mu.RLock() - var instancesToStart []*instance.Process - var instancesToStop []*instance.Process + var instancesToStart []*instance.Instance + var instancesToStop []*instance.Instance for _, inst := range im.instances { if inst.IsRunning() && // Was running when persisted inst.GetOptions() != nil && @@ -374,7 +374,7 @@ func (im *instanceManager) onStatusChange(name string, oldStatus, newStatus inst // getNodeForInstance returns the node configuration for a remote instance // Returns nil if the instance is not remote or the node is not found -func (im *instanceManager) getNodeForInstance(inst *instance.Process) *config.NodeConfig { +func (im *instanceManager) getNodeForInstance(inst *instance.Instance) *config.NodeConfig { if !inst.IsRemote() { return nil } diff --git a/pkg/manager/operations.go b/pkg/manager/operations.go index a8b5c3f..b33c34c 100644 --- a/pkg/manager/operations.go +++ b/pkg/manager/operations.go @@ -14,7 +14,7 @@ type MaxRunningInstancesError error // updateLocalInstanceFromRemote updates the local stub instance with data from the remote instance // while preserving the Nodes field to maintain remote instance tracking -func (im *instanceManager) updateLocalInstanceFromRemote(localInst *instance.Process, remoteInst *instance.Process) { +func (im *instanceManager) updateLocalInstanceFromRemote(localInst *instance.Instance, remoteInst *instance.Instance) { if localInst == nil || remoteInst == nil { return } @@ -45,9 +45,9 @@ func (im *instanceManager) updateLocalInstanceFromRemote(localInst *instance.Pro // ListInstances returns a list of all instances managed by the instance manager. // For remote instances, this fetches the live state from remote nodes and updates local stubs. -func (im *instanceManager) ListInstances() ([]*instance.Process, error) { +func (im *instanceManager) ListInstances() ([]*instance.Instance, error) { im.mu.RLock() - localInstances := make([]*instance.Process, 0, len(im.instances)) + localInstances := make([]*instance.Instance, 0, len(im.instances)) for _, inst := range im.instances { localInstances = append(localInstances, inst) } @@ -75,7 +75,7 @@ func (im *instanceManager) ListInstances() ([]*instance.Process, error) { // CreateInstance creates a new instance with the given options and returns it. // The instance is initially in a "stopped" state. -func (im *instanceManager) CreateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Process, error) { +func (im *instanceManager) CreateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { if options == nil { return nil, fmt.Errorf("instance options cannot be nil") } @@ -164,7 +164,7 @@ func (im *instanceManager) CreateInstance(name string, options *instance.CreateI // GetInstance retrieves an instance by its name. // For remote instances, this fetches the live state from the remote node and updates the local stub. -func (im *instanceManager) GetInstance(name string) (*instance.Process, error) { +func (im *instanceManager) GetInstance(name string) (*instance.Instance, error) { im.mu.RLock() inst, exists := im.instances[name] im.mu.RUnlock() @@ -194,7 +194,7 @@ func (im *instanceManager) GetInstance(name string) (*instance.Process, error) { // UpdateInstance updates the options of an existing instance and returns it. // If the instance is running, it will be restarted to apply the new options. -func (im *instanceManager) UpdateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Process, error) { +func (im *instanceManager) UpdateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { im.mu.RLock() inst, exists := im.instances[name] im.mu.RUnlock() @@ -326,7 +326,7 @@ func (im *instanceManager) DeleteInstance(name string) error { // StartInstance starts a stopped instance and returns it. // If the instance is already running, it returns an error. -func (im *instanceManager) StartInstance(name string) (*instance.Process, error) { +func (im *instanceManager) StartInstance(name string) (*instance.Instance, error) { im.mu.RLock() inst, exists := im.instances[name] im.mu.RUnlock() @@ -395,7 +395,7 @@ func (im *instanceManager) IsMaxRunningInstancesReached() bool { } // StopInstance stops a running instance and returns it. -func (im *instanceManager) StopInstance(name string) (*instance.Process, error) { +func (im *instanceManager) StopInstance(name string) (*instance.Instance, error) { im.mu.RLock() inst, exists := im.instances[name] im.mu.RUnlock() @@ -438,7 +438,7 @@ func (im *instanceManager) StopInstance(name string) (*instance.Process, error) } // RestartInstance stops and then starts an instance, returning the updated instance. -func (im *instanceManager) RestartInstance(name string) (*instance.Process, error) { +func (im *instanceManager) RestartInstance(name string) (*instance.Instance, error) { im.mu.RLock() inst, exists := im.instances[name] im.mu.RUnlock() diff --git a/pkg/manager/remote_ops.go b/pkg/manager/remote_ops.go index 40b2384..9e25623 100644 --- a/pkg/manager/remote_ops.go +++ b/pkg/manager/remote_ops.go @@ -87,13 +87,13 @@ func parseRemoteResponse(resp *http.Response, result any) error { } // ListRemoteInstances lists all instances on the remote node -func (im *instanceManager) ListRemoteInstances(nodeConfig *config.NodeConfig) ([]*instance.Process, error) { +func (im *instanceManager) ListRemoteInstances(nodeConfig *config.NodeConfig) ([]*instance.Instance, error) { resp, err := im.makeRemoteRequest(nodeConfig, "GET", "/api/v1/instances/", nil) if err != nil { return nil, err } - var instances []*instance.Process + var instances []*instance.Instance if err := parseRemoteResponse(resp, &instances); err != nil { return nil, err } @@ -102,7 +102,7 @@ func (im *instanceManager) ListRemoteInstances(nodeConfig *config.NodeConfig) ([ } // CreateRemoteInstance creates a new instance on the remote node -func (im *instanceManager) CreateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Process, error) { +func (im *instanceManager) CreateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/", name) resp, err := im.makeRemoteRequest(nodeConfig, "POST", path, options) @@ -110,7 +110,7 @@ func (im *instanceManager) CreateRemoteInstance(nodeConfig *config.NodeConfig, n return nil, err } - var inst instance.Process + var inst instance.Instance if err := parseRemoteResponse(resp, &inst); err != nil { return nil, err } @@ -119,14 +119,14 @@ func (im *instanceManager) CreateRemoteInstance(nodeConfig *config.NodeConfig, n } // GetRemoteInstance retrieves an instance by name from the remote node -func (im *instanceManager) GetRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Process, error) { +func (im *instanceManager) GetRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/", name) resp, err := im.makeRemoteRequest(nodeConfig, "GET", path, nil) if err != nil { return nil, err } - var inst instance.Process + var inst instance.Instance if err := parseRemoteResponse(resp, &inst); err != nil { return nil, err } @@ -135,7 +135,7 @@ func (im *instanceManager) GetRemoteInstance(nodeConfig *config.NodeConfig, name } // UpdateRemoteInstance updates an existing instance on the remote node -func (im *instanceManager) UpdateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Process, error) { +func (im *instanceManager) UpdateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/", name) resp, err := im.makeRemoteRequest(nodeConfig, "PUT", path, options) @@ -143,7 +143,7 @@ func (im *instanceManager) UpdateRemoteInstance(nodeConfig *config.NodeConfig, n return nil, err } - var inst instance.Process + var inst instance.Instance if err := parseRemoteResponse(resp, &inst); err != nil { return nil, err } @@ -163,14 +163,14 @@ func (im *instanceManager) DeleteRemoteInstance(nodeConfig *config.NodeConfig, n } // StartRemoteInstance starts an instance on the remote node -func (im *instanceManager) StartRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Process, error) { +func (im *instanceManager) StartRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/start", name) resp, err := im.makeRemoteRequest(nodeConfig, "POST", path, nil) if err != nil { return nil, err } - var inst instance.Process + var inst instance.Instance if err := parseRemoteResponse(resp, &inst); err != nil { return nil, err } @@ -179,14 +179,14 @@ func (im *instanceManager) StartRemoteInstance(nodeConfig *config.NodeConfig, na } // StopRemoteInstance stops an instance on the remote node -func (im *instanceManager) StopRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Process, error) { +func (im *instanceManager) StopRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/stop", name) resp, err := im.makeRemoteRequest(nodeConfig, "POST", path, nil) if err != nil { return nil, err } - var inst instance.Process + var inst instance.Instance if err := parseRemoteResponse(resp, &inst); err != nil { return nil, err } @@ -195,14 +195,14 @@ func (im *instanceManager) StopRemoteInstance(nodeConfig *config.NodeConfig, nam } // RestartRemoteInstance restarts an instance on the remote node -func (im *instanceManager) RestartRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Process, error) { +func (im *instanceManager) RestartRemoteInstance(nodeConfig *config.NodeConfig, name string) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/restart", name) resp, err := im.makeRemoteRequest(nodeConfig, "POST", path, nil) if err != nil { return nil, err } - var inst instance.Process + var inst instance.Instance if err := parseRemoteResponse(resp, &inst); err != nil { return nil, err } diff --git a/pkg/manager/timeout.go b/pkg/manager/timeout.go index 50b1c10..2e0314a 100644 --- a/pkg/manager/timeout.go +++ b/pkg/manager/timeout.go @@ -37,7 +37,7 @@ func (im *instanceManager) checkAllTimeouts() { // EvictLRUInstance finds and stops the least recently used running instance. func (im *instanceManager) EvictLRUInstance() error { im.mu.RLock() - var lruInstance *instance.Process + var lruInstance *instance.Instance for name := range im.runningInstances { inst := im.instances[name] diff --git a/pkg/manager/timeout_test.go b/pkg/manager/timeout_test.go index 55cd781..214a488 100644 --- a/pkg/manager/timeout_test.go +++ b/pkg/manager/timeout_test.go @@ -196,7 +196,7 @@ func TestEvictLRUInstance_Success(t *testing.T) { func TestEvictLRUInstance_NoEligibleInstances(t *testing.T) { // Helper function to create instances with different timeout configurations - createInstanceWithTimeout := func(manager manager.InstanceManager, name, model string, timeout *int) *instance.Process { + createInstanceWithTimeout := func(manager manager.InstanceManager, name, model string, timeout *int) *instance.Instance { options := &instance.CreateInstanceOptions{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -236,7 +236,7 @@ func TestEvictLRUInstance_NoEligibleInstances(t *testing.T) { inst3 := createInstanceWithTimeout(manager, "no-timeout-3", "/path/to/model3.gguf", nil) // Set instances to running - instances := []*instance.Process{inst1, inst2, inst3} + instances := []*instance.Instance{inst1, inst2, inst3} for _, inst := range instances { inst.SetStatus(instance.Running) } @@ -276,7 +276,7 @@ func TestEvictLRUInstance_NoEligibleInstances(t *testing.T) { instNoTimeout2 := createInstanceWithTimeout(manager, "no-timeout-2", "/path/to/model-no-timeout2.gguf", nil) // Set all instances to running - instances := []*instance.Process{instWithTimeout, instNoTimeout1, instNoTimeout2} + instances := []*instance.Instance{instWithTimeout, instNoTimeout1, instNoTimeout2} for _, inst := range instances { inst.SetStatus(instance.Running) inst.UpdateLastRequestTime() diff --git a/pkg/server/handlers_instances.go b/pkg/server/handlers_instances.go index be3cf4a..080952c 100644 --- a/pkg/server/handlers_instances.go +++ b/pkg/server/handlers_instances.go @@ -391,7 +391,7 @@ func (h *Handler) ProxyToInstance() http.HandlerFunc { } // RemoteInstanceProxy proxies requests to a remote instance -func (h *Handler) RemoteInstanceProxy(w http.ResponseWriter, r *http.Request, name string, inst *instance.Process) { +func (h *Handler) RemoteInstanceProxy(w http.ResponseWriter, r *http.Request, name string, inst *instance.Instance) { // Get the node name from instance options options := inst.GetOptions() if options == nil || len(options.Nodes) == 0 { diff --git a/pkg/server/handlers_openai.go b/pkg/server/handlers_openai.go index c6e56e9..2849841 100644 --- a/pkg/server/handlers_openai.go +++ b/pkg/server/handlers_openai.go @@ -152,7 +152,7 @@ func (h *Handler) OpenAIProxy() http.HandlerFunc { } // RemoteOpenAIProxy proxies OpenAI-compatible requests to a remote instance -func (h *Handler) RemoteOpenAIProxy(w http.ResponseWriter, r *http.Request, modelName string, inst *instance.Process) { +func (h *Handler) RemoteOpenAIProxy(w http.ResponseWriter, r *http.Request, modelName string, inst *instance.Instance) { // Get the node name from instance options options := inst.GetOptions() if options == nil || len(options.Nodes) == 0 { From e0ec00d141ee3ca1f75689a6011c90691613aad7 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Thu, 16 Oct 2025 19:40:03 +0200 Subject: [PATCH 06/16] Remove rendundant instance prefix from status --- pkg/instance/instance.go | 6 +++--- pkg/instance/instance_test.go | 12 ++++++------ pkg/instance/status.go | 16 ++++++++-------- pkg/instance/timeout_test.go | 12 ++++++------ pkg/manager/manager.go | 6 +++--- pkg/manager/operations.go | 2 +- 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 605fd59..5123dab 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -22,8 +22,8 @@ type Instance struct { globalBackendSettings *config.BackendConfig // Status - Status InstanceStatus `json:"status"` - onStatusChange func(oldStatus, newStatus InstanceStatus) + Status Status `json:"status"` + onStatusChange func(oldStatus, newStatus Status) // Creation time Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created @@ -49,7 +49,7 @@ type Instance struct { } // NewInstance creates a new instance with the given name, log path, and options -func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, options *CreateInstanceOptions, onStatusChange func(oldStatus, newStatus InstanceStatus)) *Instance { +func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, options *CreateInstanceOptions, onStatusChange func(oldStatus, newStatus Status)) *Instance { // Validate and copy options options.ValidateAndApplyDefaults(name, globalInstanceSettings) diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index f22ee72..9940633 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -42,7 +42,7 @@ func TestNewInstance(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) @@ -113,7 +113,7 @@ func TestNewInstance_WithRestartOptions(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} instance := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) opts := instance.GetOptions() @@ -162,7 +162,7 @@ func TestSetOptions(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, initialOptions, mockOnStatusChange) @@ -220,7 +220,7 @@ func TestGetProxy(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) @@ -275,7 +275,7 @@ func TestMarshalJSON(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} instance := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) @@ -444,7 +444,7 @@ func TestCreateInstanceOptionsValidation(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} instance := instance.NewInstance("test", backendConfig, globalSettings, options, mockOnStatusChange) opts := instance.GetOptions() diff --git a/pkg/instance/status.go b/pkg/instance/status.go index b06bef0..2c1045e 100644 --- a/pkg/instance/status.go +++ b/pkg/instance/status.go @@ -6,27 +6,27 @@ import ( ) // Enum for instance status -type InstanceStatus int +type Status int const ( - Stopped InstanceStatus = iota + Stopped Status = iota Running Failed ) -var nameToStatus = map[string]InstanceStatus{ +var nameToStatus = map[string]Status{ "stopped": Stopped, "running": Running, "failed": Failed, } -var statusToName = map[InstanceStatus]string{ +var statusToName = map[Status]string{ Stopped: "stopped", Running: "running", Failed: "failed", } -func (p *Instance) SetStatus(status InstanceStatus) { +func (p *Instance) SetStatus(status Status) { oldStatus := p.Status p.Status = status @@ -35,7 +35,7 @@ func (p *Instance) SetStatus(status InstanceStatus) { } } -func (p *Instance) GetStatus() InstanceStatus { +func (p *Instance) GetStatus() Status { return p.Status } @@ -44,7 +44,7 @@ func (p *Instance) IsRunning() bool { return p.Status == Running } -func (s InstanceStatus) MarshalJSON() ([]byte, error) { +func (s Status) MarshalJSON() ([]byte, error) { name, ok := statusToName[s] if !ok { name = "stopped" // Default to "stopped" for unknown status @@ -53,7 +53,7 @@ func (s InstanceStatus) MarshalJSON() ([]byte, error) { } // UnmarshalJSON implements json.Unmarshaler -func (s *InstanceStatus) UnmarshalJSON(data []byte) error { +func (s *Status) UnmarshalJSON(data []byte) error { var str string if err := json.Unmarshal(data, &str); err != nil { return err diff --git a/pkg/instance/timeout_test.go b/pkg/instance/timeout_test.go index 1171c6a..c602131 100644 --- a/pkg/instance/timeout_test.go +++ b/pkg/instance/timeout_test.go @@ -54,7 +54,7 @@ func TestUpdateLastRequestTime(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) @@ -86,7 +86,7 @@ func TestShouldTimeout_NotRunning(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) @@ -122,7 +122,7 @@ func TestShouldTimeout_NoTimeoutConfigured(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} options := &instance.CreateInstanceOptions{ IdleTimeout: tt.idleTimeout, @@ -167,7 +167,7 @@ func TestShouldTimeout_WithinTimeLimit(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) inst.SetStatus(instance.Running) @@ -205,7 +205,7 @@ func TestShouldTimeout_ExceedsTimeLimit(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) inst.SetStatus(instance.Running) @@ -261,7 +261,7 @@ func TestTimeoutConfiguration_Validation(t *testing.T) { } // Mock onStatusChange function - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, mockOnStatusChange) opts := inst.GetOptions() diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 258d4b7..12e1a59 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -276,10 +276,10 @@ func (im *instanceManager) loadInstance(name, path string) error { // Check if this is a remote instance isRemote := options != nil && len(options.Nodes) > 0 - var statusCallback func(oldStatus, newStatus instance.InstanceStatus) + var statusCallback func(oldStatus, newStatus instance.Status) if !isRemote { // Only set status callback for local instances - statusCallback = func(oldStatus, newStatus instance.InstanceStatus) { + statusCallback = func(oldStatus, newStatus instance.Status) { im.onStatusChange(persistedInstance.Name, oldStatus, newStatus) } } @@ -361,7 +361,7 @@ func (im *instanceManager) autoStartInstances() { } } -func (im *instanceManager) onStatusChange(name string, oldStatus, newStatus instance.InstanceStatus) { +func (im *instanceManager) onStatusChange(name string, oldStatus, newStatus instance.Status) { im.mu.Lock() defer im.mu.Unlock() diff --git a/pkg/manager/operations.go b/pkg/manager/operations.go index b33c34c..80858d0 100644 --- a/pkg/manager/operations.go +++ b/pkg/manager/operations.go @@ -148,7 +148,7 @@ func (im *instanceManager) CreateInstance(name string, options *instance.CreateI return nil, err } - statusCallback := func(oldStatus, newStatus instance.InstanceStatus) { + statusCallback := func(oldStatus, newStatus instance.Status) { im.onStatusChange(name, oldStatus, newStatus) } From 5afc22924f422786b6a1a01e971249c8f7d568db Mon Sep 17 00:00:00 2001 From: LordMathis Date: Thu, 16 Oct 2025 20:15:22 +0200 Subject: [PATCH 07/16] Refactor Status struct --- pkg/instance/instance.go | 88 +++++++++++++++++++++++++++------------ pkg/instance/status.go | 82 +++++++++++++++++++++++++++--------- pkg/manager/manager.go | 2 +- pkg/manager/operations.go | 2 +- 4 files changed, 125 insertions(+), 49 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 5123dab..0480062 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -16,23 +16,21 @@ import ( // Instance represents a running instance of the llama server type Instance struct { - Name string `json:"name"` - options *CreateInstanceOptions `json:"-"` + // Immutable identity (no locking needed after creation) + Name string `json:"name"` + Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created + + // Mutable state - each owns its own lock + status *status `json:"status"` // unexported - status owns its lock + options *CreateInstanceOptions `json:"-"` + + // Global configuration (read-only, no lock needed) globalInstanceSettings *config.InstancesConfig globalBackendSettings *config.BackendConfig - // Status - Status Status `json:"status"` - onStatusChange func(oldStatus, newStatus Status) - - // Creation time - Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created - - // Logging file - logger *logger `json:"-"` - - // Proxy component - proxy *proxy `json:"-"` // HTTP proxy and request tracking + // Components (can be nil for remote instances or when stopped) + logger *logger `json:"-"` // nil for remote instances + proxy *proxy `json:"-"` // nil for remote instances // internal cmd *exec.Cmd `json:"-"` // Command to run the instance @@ -56,6 +54,10 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa // Create the instance logger logger := NewLogger(name, globalInstanceSettings.LogsDir) + // Create status wrapper + status := newStatus(Stopped) + status.onStatusChange = onStatusChange + instance := &Instance{ Name: name, options: options, @@ -63,8 +65,7 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa globalBackendSettings: globalBackendSettings, logger: logger, Created: time.Now().Unix(), - Status: Stopped, - onStatusChange: onStatusChange, + status: status, } // Create Proxy component @@ -79,6 +80,29 @@ func (i *Instance) GetOptions() *CreateInstanceOptions { return i.options } +// GetStatus returns the current status, delegating to status component +func (i *Instance) GetStatus() Status { + if i.status == nil { + return Stopped + } + return i.status.Get() +} + +// SetStatus sets the status, delegating to status component +func (i *Instance) SetStatus(s Status) { + if i.status != nil { + i.status.Set(s) + } +} + +// IsRunning returns true if the status is Running, delegating to status component +func (i *Instance) IsRunning() bool { + if i.status == nil { + return false + } + return i.status.IsRunning() +} + func (i *Instance) GetPort() int { i.mu.RLock() defer i.mu.RUnlock() @@ -182,14 +206,17 @@ func (i *Instance) MarshalJSON() ([]byte, error) { } } - // Use anonymous struct to avoid recursion - type Alias Instance + // Explicitly serialize to maintain backward compatible JSON format return json.Marshal(&struct { - *Alias + Name string `json:"name"` + Status *status `json:"status"` + Created int64 `json:"created,omitempty"` Options *CreateInstanceOptions `json:"options,omitempty"` DockerEnabled bool `json:"docker_enabled,omitempty"` }{ - Alias: (*Alias)(i), + Name: i.Name, + Status: i.status, + Created: i.Created, Options: i.options, DockerEnabled: dockerEnabled, }) @@ -197,26 +224,33 @@ func (i *Instance) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler for Instance func (i *Instance) UnmarshalJSON(data []byte) error { - // Use anonymous struct to avoid recursion - type Alias Instance + // Explicitly deserialize to match MarshalJSON format aux := &struct { - *Alias + Name string `json:"name"` + Status *status `json:"status"` + Created int64 `json:"created,omitempty"` Options *CreateInstanceOptions `json:"options,omitempty"` - }{ - Alias: (*Alias)(i), - } + }{} if err := json.Unmarshal(data, aux); err != nil { return err } + // Set the fields + i.Name = aux.Name + i.Created = aux.Created + i.status = aux.Status + // Handle options with validation and defaults if aux.Options != nil { aux.Options.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) i.options = aux.Options } - // Initialize fields that are not serialized + // Initialize fields that are not serialized or may be nil + if i.status == nil { + i.status = newStatus(Stopped) + } if i.logger == nil && i.globalInstanceSettings != nil { i.logger = NewLogger(i.Name, i.globalInstanceSettings.LogsDir) } diff --git a/pkg/instance/status.go b/pkg/instance/status.go index 2c1045e..8c24f49 100644 --- a/pkg/instance/status.go +++ b/pkg/instance/status.go @@ -3,9 +3,10 @@ package instance import ( "encoding/json" "log" + "sync" ) -// Enum for instance status +// Status is the enum for status values (exported). type Status int const ( @@ -26,24 +27,7 @@ var statusToName = map[Status]string{ Failed: "failed", } -func (p *Instance) SetStatus(status Status) { - oldStatus := p.Status - p.Status = status - - if p.onStatusChange != nil { - p.onStatusChange(oldStatus, status) - } -} - -func (p *Instance) GetStatus() Status { - return p.Status -} - -// IsRunning returns true if the status is Running -func (p *Instance) IsRunning() bool { - return p.Status == Running -} - +// Status enum JSON marshaling methods func (s Status) MarshalJSON() ([]byte, error) { name, ok := statusToName[s] if !ok { @@ -52,7 +36,7 @@ func (s Status) MarshalJSON() ([]byte, error) { return json.Marshal(name) } -// UnmarshalJSON implements json.Unmarshaler +// UnmarshalJSON implements json.Unmarshaler for Status enum func (s *Status) UnmarshalJSON(data []byte) error { var str string if err := json.Unmarshal(data, &str); err != nil { @@ -68,3 +52,61 @@ func (s *Status) UnmarshalJSON(data []byte) error { *s = status return nil } + +// status represents the instance status with thread-safe access (unexported). +type status struct { + mu sync.RWMutex + s Status + + // Callback for status changes + onStatusChange func(oldStatus, newStatus Status) +} + +// newStatus creates a new status wrapper with the given initial status +func newStatus(initial Status) *status { + return &status{ + s: initial, + } +} + +// Get returns the current status +func (st *status) Get() Status { + st.mu.RLock() + defer st.mu.RUnlock() + return st.s +} + +// Set updates the status and triggers the onStatusChange callback if set +func (st *status) Set(newStatus Status) { + st.mu.Lock() + oldStatus := st.s + st.s = newStatus + callback := st.onStatusChange + st.mu.Unlock() + + // Call the callback outside the lock to avoid potential deadlocks + if callback != nil { + callback(oldStatus, newStatus) + } +} + +// IsRunning returns true if the status is Running +func (st *status) IsRunning() bool { + st.mu.RLock() + defer st.mu.RUnlock() + return st.s == Running +} + +// MarshalJSON implements json.Marshaler for status wrapper +func (st *status) MarshalJSON() ([]byte, error) { + st.mu.RLock() + defer st.mu.RUnlock() + return st.s.MarshalJSON() +} + +// UnmarshalJSON implements json.Unmarshaler for status wrapper +func (st *status) UnmarshalJSON(data []byte) error { + st.mu.Lock() + defer st.mu.Unlock() + return st.s.UnmarshalJSON(data) +} diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 12e1a59..da5a900 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -289,7 +289,7 @@ func (im *instanceManager) loadInstance(name, path string) error { // Restore persisted fields that NewInstance doesn't set inst.Created = persistedInstance.Created - inst.SetStatus(persistedInstance.Status) + inst.SetStatus(persistedInstance.GetStatus()) // Handle remote instance mapping if isRemote { diff --git a/pkg/manager/operations.go b/pkg/manager/operations.go index 80858d0..cc2dba4 100644 --- a/pkg/manager/operations.go +++ b/pkg/manager/operations.go @@ -39,7 +39,7 @@ func (im *instanceManager) updateLocalInstanceFromRemote(localInst *instance.Ins // Update the local instance with all remote data localInst.SetOptions(&updatedOptions) - localInst.Status = remoteInst.Status + localInst.SetStatus(remoteInst.GetStatus()) localInst.Created = remoteInst.Created } From a96ed4d797a4a4ef713b2961092d6b66b340b456 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Thu, 16 Oct 2025 20:22:12 +0200 Subject: [PATCH 08/16] Fix status json tag static check --- pkg/instance/instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 0480062..fba8b8c 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -21,7 +21,7 @@ type Instance struct { Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created // Mutable state - each owns its own lock - status *status `json:"status"` // unexported - status owns its lock + status *status `json:"-"` // unexported - status owns its lock options *CreateInstanceOptions `json:"-"` // Global configuration (read-only, no lock needed) From 4b30791be2c4700942ef19b702310a80b18705ae Mon Sep 17 00:00:00 2001 From: LordMathis Date: Thu, 16 Oct 2025 20:53:24 +0200 Subject: [PATCH 09/16] Refactor instance options structure and related code --- pkg/instance/instance.go | 160 ++++++++++++++++-------------- pkg/instance/instance_test.go | 14 +-- pkg/instance/lifecycle.go | 34 +++++-- pkg/instance/options.go | 69 +++++++++++-- pkg/instance/timeout_test.go | 12 +-- pkg/manager/manager.go | 8 +- pkg/manager/manager_test.go | 8 +- pkg/manager/operations.go | 10 +- pkg/manager/operations_test.go | 14 +-- pkg/manager/remote_ops.go | 8 +- pkg/manager/remote_ops_test.go | 2 +- pkg/manager/timeout_test.go | 12 +-- pkg/server/handlers_backends.go | 12 +-- pkg/server/handlers_instances.go | 8 +- pkg/validation/validation.go | 8 +- pkg/validation/validation_test.go | 16 +-- 16 files changed, 235 insertions(+), 160 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index fba8b8c..6a86fcd 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -21,8 +21,8 @@ type Instance struct { Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created // Mutable state - each owns its own lock - status *status `json:"-"` // unexported - status owns its lock - options *CreateInstanceOptions `json:"-"` + status *status `json:"-"` // unexported - status owns its lock + options *options `json:"-"` // unexported - options owns its lock // Global configuration (read-only, no lock needed) globalInstanceSettings *config.InstancesConfig @@ -47,9 +47,9 @@ type Instance struct { } // NewInstance creates a new instance with the given name, log path, and options -func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, options *CreateInstanceOptions, onStatusChange func(oldStatus, newStatus Status)) *Instance { +func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, opts *Options, onStatusChange func(oldStatus, newStatus Status)) *Instance { // Validate and copy options - options.ValidateAndApplyDefaults(name, globalInstanceSettings) + opts.ValidateAndApplyDefaults(name, globalInstanceSettings) // Create the instance logger logger := NewLogger(name, globalInstanceSettings.LogsDir) @@ -58,6 +58,9 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa status := newStatus(Stopped) status.onStatusChange = onStatusChange + // Create options wrapper + options := newOptions(opts) + instance := &Instance{ Name: name, options: options, @@ -74,10 +77,12 @@ func NewInstance(name string, globalBackendSettings *config.BackendConfig, globa return instance } -func (i *Instance) GetOptions() *CreateInstanceOptions { - i.mu.RLock() - defer i.mu.RUnlock() - return i.options +// GetOptions returns the current options, delegating to options component +func (i *Instance) GetOptions() *Options { + if i.options == nil { + return nil + } + return i.options.Get() } // GetStatus returns the current status, delegating to status component @@ -104,21 +109,20 @@ func (i *Instance) IsRunning() bool { } func (i *Instance) GetPort() int { - i.mu.RLock() - defer i.mu.RUnlock() - if i.options != nil { - switch i.options.BackendType { + opts := i.GetOptions() + if opts != nil { + switch opts.BackendType { case backends.BackendTypeLlamaCpp: - if i.options.LlamaServerOptions != nil { - return i.options.LlamaServerOptions.Port + if opts.LlamaServerOptions != nil { + return opts.LlamaServerOptions.Port } case backends.BackendTypeMlxLm: - if i.options.MlxServerOptions != nil { - return i.options.MlxServerOptions.Port + if opts.MlxServerOptions != nil { + return opts.MlxServerOptions.Port } case backends.BackendTypeVllm: - if i.options.VllmServerOptions != nil { - return i.options.VllmServerOptions.Port + if opts.VllmServerOptions != nil { + return opts.VllmServerOptions.Port } } } @@ -126,40 +130,39 @@ func (i *Instance) GetPort() int { } func (i *Instance) GetHost() string { - i.mu.RLock() - defer i.mu.RUnlock() - if i.options != nil { - switch i.options.BackendType { + opts := i.GetOptions() + if opts != nil { + switch opts.BackendType { case backends.BackendTypeLlamaCpp: - if i.options.LlamaServerOptions != nil { - return i.options.LlamaServerOptions.Host + if opts.LlamaServerOptions != nil { + return opts.LlamaServerOptions.Host } case backends.BackendTypeMlxLm: - if i.options.MlxServerOptions != nil { - return i.options.MlxServerOptions.Host + if opts.MlxServerOptions != nil { + return opts.MlxServerOptions.Host } case backends.BackendTypeVllm: - if i.options.VllmServerOptions != nil { - return i.options.VllmServerOptions.Host + if opts.VllmServerOptions != nil { + return opts.VllmServerOptions.Host } } } return "" } -func (i *Instance) SetOptions(options *CreateInstanceOptions) { - i.mu.Lock() - defer i.mu.Unlock() - - if options == nil { +// SetOptions sets the options, delegating to options component +func (i *Instance) SetOptions(opts *Options) { + if opts == nil { log.Println("Warning: Attempted to set nil options on instance", i.Name) return } // Validate and copy options - options.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) + opts.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) - i.options = options + if i.options != nil { + i.options.Set(opts) + } // Clear the proxy so it gets recreated with new options if i.proxy != nil { @@ -189,10 +192,13 @@ func (i *Instance) MarshalJSON() ([]byte, error) { i.mu.RLock() defer i.mu.RUnlock() + // Get options + opts := i.GetOptions() + // Determine if docker is enabled for this instance's backend var dockerEnabled bool - if i.options != nil { - switch i.options.BackendType { + if opts != nil { + switch opts.BackendType { case backends.BackendTypeLlamaCpp: if i.globalBackendSettings != nil && i.globalBackendSettings.LlamaCpp.Docker != nil && i.globalBackendSettings.LlamaCpp.Docker.Enabled { dockerEnabled = true @@ -208,11 +214,11 @@ func (i *Instance) MarshalJSON() ([]byte, error) { // Explicitly serialize to maintain backward compatible JSON format return json.Marshal(&struct { - Name string `json:"name"` - Status *status `json:"status"` - Created int64 `json:"created,omitempty"` - Options *CreateInstanceOptions `json:"options,omitempty"` - DockerEnabled bool `json:"docker_enabled,omitempty"` + Name string `json:"name"` + Status *status `json:"status"` + Created int64 `json:"created,omitempty"` + Options *options `json:"options,omitempty"` + DockerEnabled bool `json:"docker_enabled,omitempty"` }{ Name: i.Name, Status: i.status, @@ -226,10 +232,10 @@ func (i *Instance) MarshalJSON() ([]byte, error) { func (i *Instance) UnmarshalJSON(data []byte) error { // Explicitly deserialize to match MarshalJSON format aux := &struct { - Name string `json:"name"` - Status *status `json:"status"` - Created int64 `json:"created,omitempty"` - Options *CreateInstanceOptions `json:"options,omitempty"` + Name string `json:"name"` + Status *status `json:"status"` + Created int64 `json:"created,omitempty"` + Options *options `json:"options,omitempty"` }{} if err := json.Unmarshal(data, aux); err != nil { @@ -240,69 +246,79 @@ func (i *Instance) UnmarshalJSON(data []byte) error { i.Name = aux.Name i.Created = aux.Created i.status = aux.Status + i.options = aux.Options // Handle options with validation and defaults - if aux.Options != nil { - aux.Options.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) - i.options = aux.Options + if i.options != nil { + opts := i.options.Get() + if opts != nil { + opts.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) + } } // Initialize fields that are not serialized or may be nil if i.status == nil { i.status = newStatus(Stopped) } - if i.logger == nil && i.globalInstanceSettings != nil { - i.logger = NewLogger(i.Name, i.globalInstanceSettings.LogsDir) + if i.options == nil { + i.options = newOptions(&Options{}) } - if i.proxy == nil { - i.proxy = NewProxy(i) + + // Only create logger and proxy for non-remote instances + // Remote instances are metadata only (no logger, proxy, or process) + if !i.IsRemote() { + if i.logger == nil && i.globalInstanceSettings != nil { + i.logger = NewLogger(i.Name, i.globalInstanceSettings.LogsDir) + } + if i.proxy == nil { + i.proxy = NewProxy(i) + } } return nil } func (i *Instance) IsRemote() bool { - i.mu.RLock() - defer i.mu.RUnlock() - - if i.options == nil { + opts := i.GetOptions() + if opts == nil { return false } - return len(i.options.Nodes) > 0 + return len(opts.Nodes) > 0 } func (i *Instance) GetLogs(num_lines int) (string, error) { + if i.logger == nil { + return "", fmt.Errorf("instance %s has no logger (remote instances don't have logs)", i.Name) + } return i.logger.GetLogs(num_lines) } // getBackendHostPort extracts the host and port from instance options // Returns the configured host and port for the backend func (i *Instance) getBackendHostPort() (string, int) { - i.mu.RLock() - defer i.mu.RUnlock() - - if i.options == nil { + opts := i.GetOptions() + if opts == nil { return "localhost", 0 } var host string var port int - switch i.options.BackendType { + switch opts.BackendType { case backends.BackendTypeLlamaCpp: - if i.options.LlamaServerOptions != nil { - host = i.options.LlamaServerOptions.Host - port = i.options.LlamaServerOptions.Port + if opts.LlamaServerOptions != nil { + host = opts.LlamaServerOptions.Host + port = opts.LlamaServerOptions.Port } case backends.BackendTypeMlxLm: - if i.options.MlxServerOptions != nil { - host = i.options.MlxServerOptions.Host - port = i.options.MlxServerOptions.Port + if opts.MlxServerOptions != nil { + host = opts.MlxServerOptions.Host + port = opts.MlxServerOptions.Port } case backends.BackendTypeVllm: - if i.options.VllmServerOptions != nil { - host = i.options.VllmServerOptions.Host - port = i.options.VllmServerOptions.Port + if opts.VllmServerOptions != nil { + host = opts.VllmServerOptions.Host + port = opts.VllmServerOptions.Port } } diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index 9940633..d2c31ce 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -33,7 +33,7 @@ func TestNewInstance(t *testing.T) { DefaultRestartDelay: 5, } - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -102,7 +102,7 @@ func TestNewInstance_WithRestartOptions(t *testing.T) { maxRestarts := 10 restartDelay := 15 - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ AutoRestart: &autoRestart, MaxRestarts: &maxRestarts, RestartDelay: &restartDelay, @@ -153,7 +153,7 @@ func TestSetOptions(t *testing.T) { DefaultRestartDelay: 5, } - initialOptions := &instance.CreateInstanceOptions{ + initialOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -167,7 +167,7 @@ func TestSetOptions(t *testing.T) { inst := instance.NewInstance("test-instance", backendConfig, globalSettings, initialOptions, mockOnStatusChange) // Update options - newOptions := &instance.CreateInstanceOptions{ + newOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/new-model.gguf", @@ -211,7 +211,7 @@ func TestGetProxy(t *testing.T) { LogsDir: "/tmp/test", } - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Host: "localhost", @@ -266,7 +266,7 @@ func TestMarshalJSON(t *testing.T) { DefaultRestartDelay: 5, } - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -434,7 +434,7 @@ func TestCreateInstanceOptionsValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ MaxRestarts: tt.maxRestarts, RestartDelay: tt.restartDelay, BackendType: backends.BackendTypeLlamaCpp, diff --git a/pkg/instance/lifecycle.go b/pkg/instance/lifecycle.go index 23bd385..86f73db 100644 --- a/pkg/instance/lifecycle.go +++ b/pkg/instance/lifecycle.go @@ -311,29 +311,30 @@ func (i *Instance) handleRestart() { // validateRestartConditions checks if the instance should be restarted and returns the parameters func (i *Instance) validateRestartConditions() (shouldRestart bool, maxRestarts int, restartDelay int) { - if i.options == nil { + opts := i.GetOptions() + if opts == nil { log.Printf("Instance %s not restarting: options are nil", i.Name) return false, 0, 0 } - if i.options.AutoRestart == nil || !*i.options.AutoRestart { + if opts.AutoRestart == nil || !*opts.AutoRestart { log.Printf("Instance %s not restarting: AutoRestart is disabled", i.Name) return false, 0, 0 } - if i.options.MaxRestarts == nil { + if opts.MaxRestarts == nil { log.Printf("Instance %s not restarting: MaxRestarts is nil", i.Name) return false, 0, 0 } - if i.options.RestartDelay == nil { + if opts.RestartDelay == nil { log.Printf("Instance %s not restarting: RestartDelay is nil", i.Name) return false, 0, 0 } // Values are already validated during unmarshaling/SetOptions - maxRestarts = *i.options.MaxRestarts - restartDelay = *i.options.RestartDelay + maxRestarts = *opts.MaxRestarts + restartDelay = *opts.RestartDelay if i.restarts >= maxRestarts { log.Printf("Instance %s exceeded max restart attempts (%d)", i.Name, maxRestarts) @@ -345,6 +346,12 @@ func (i *Instance) validateRestartConditions() (shouldRestart bool, maxRestarts // buildCommand builds the command to execute using backend-specific logic func (i *Instance) buildCommand() (*exec.Cmd, error) { + // Get options + opts := i.GetOptions() + if opts == nil { + return nil, fmt.Errorf("instance options are nil") + } + // Get backend configuration backendConfig, err := i.getBackendConfig() if err != nil { @@ -352,13 +359,13 @@ func (i *Instance) buildCommand() (*exec.Cmd, error) { } // Build the environment variables - env := i.options.BuildEnvironment(backendConfig) + env := opts.BuildEnvironment(backendConfig) // Get the command to execute - command := i.options.GetCommand(backendConfig) + command := opts.GetCommand(backendConfig) // Build command arguments - args := i.options.BuildCommandArgs(backendConfig) + args := opts.BuildCommandArgs(backendConfig) // Create the exec.Cmd cmd := exec.CommandContext(i.ctx, command, args...) @@ -376,9 +383,14 @@ func (i *Instance) buildCommand() (*exec.Cmd, error) { // getBackendConfig resolves the backend configuration for the current instance func (i *Instance) getBackendConfig() (*config.BackendSettings, error) { + opts := i.GetOptions() + if opts == nil { + return nil, fmt.Errorf("instance options are nil") + } + var backendTypeStr string - switch i.options.BackendType { + switch opts.BackendType { case backends.BackendTypeLlamaCpp: backendTypeStr = "llama-cpp" case backends.BackendTypeMlxLm: @@ -386,7 +398,7 @@ func (i *Instance) getBackendConfig() (*config.BackendSettings, error) { case backends.BackendTypeVllm: backendTypeStr = "vllm" default: - return nil, fmt.Errorf("unsupported backend type: %s", i.options.BackendType) + return nil, fmt.Errorf("unsupported backend type: %s", opts.BackendType) } settings := i.globalBackendSettings.GetBackendSettings(backendTypeStr) diff --git a/pkg/instance/options.go b/pkg/instance/options.go index 439f426..c5db58b 100644 --- a/pkg/instance/options.go +++ b/pkg/instance/options.go @@ -10,9 +10,11 @@ import ( "llamactl/pkg/config" "log" "maps" + "sync" ) -type CreateInstanceOptions struct { +// Options contains the actual configuration (exported - this is the public API). +type Options struct { // Auto restart AutoRestart *bool `json:"auto_restart,omitempty"` MaxRestarts *int `json:"max_restarts,omitempty"` @@ -35,10 +37,55 @@ type CreateInstanceOptions struct { VllmServerOptions *vllm.VllmServerOptions `json:"-"` } -// UnmarshalJSON implements custom JSON unmarshaling for CreateInstanceOptions -func (c *CreateInstanceOptions) UnmarshalJSON(data []byte) error { +// options wraps Options with thread-safe access (unexported). +type options struct { + mu sync.RWMutex + opts *Options +} + +// newOptions creates a new options wrapper with the given Options +func newOptions(opts *Options) *options { + return &options{ + opts: opts, + } +} + +// Get returns a copy of the current options +func (o *options) Get() *Options { + o.mu.RLock() + defer o.mu.RUnlock() + return o.opts +} + +// Set updates the options +func (o *options) Set(opts *Options) { + o.mu.Lock() + defer o.mu.Unlock() + o.opts = opts +} + +// MarshalJSON implements json.Marshaler for options wrapper +func (o *options) MarshalJSON() ([]byte, error) { + o.mu.RLock() + defer o.mu.RUnlock() + return o.opts.MarshalJSON() +} + +// UnmarshalJSON implements json.Unmarshaler for options wrapper +func (o *options) UnmarshalJSON(data []byte) error { + o.mu.Lock() + defer o.mu.Unlock() + + if o.opts == nil { + o.opts = &Options{} + } + return o.opts.UnmarshalJSON(data) +} + +// UnmarshalJSON implements custom JSON unmarshaling for Options +func (c *Options) UnmarshalJSON(data []byte) error { // Use anonymous struct to avoid recursion - type Alias CreateInstanceOptions + type Alias Options aux := &struct { *Alias }{ @@ -95,10 +142,10 @@ func (c *CreateInstanceOptions) UnmarshalJSON(data []byte) error { return nil } -// MarshalJSON implements custom JSON marshaling for CreateInstanceOptions -func (c *CreateInstanceOptions) MarshalJSON() ([]byte, error) { +// MarshalJSON implements custom JSON marshaling for Options +func (c *Options) MarshalJSON() ([]byte, error) { // Use anonymous struct to avoid recursion - type Alias CreateInstanceOptions + type Alias Options aux := struct { *Alias }{ @@ -155,7 +202,7 @@ func (c *CreateInstanceOptions) MarshalJSON() ([]byte, error) { } // ValidateAndApplyDefaults validates the instance options and applies constraints -func (c *CreateInstanceOptions) ValidateAndApplyDefaults(name string, globalSettings *config.InstancesConfig) { +func (c *Options) ValidateAndApplyDefaults(name string, globalSettings *config.InstancesConfig) { // Validate and apply constraints if c.MaxRestarts != nil && *c.MaxRestarts < 0 { log.Printf("Instance %s MaxRestarts value (%d) cannot be negative, setting to 0", name, *c.MaxRestarts) @@ -193,7 +240,7 @@ func (c *CreateInstanceOptions) ValidateAndApplyDefaults(name string, globalSett } } -func (c *CreateInstanceOptions) GetCommand(backendConfig *config.BackendSettings) string { +func (c *Options) GetCommand(backendConfig *config.BackendSettings) string { if backendConfig.Docker != nil && backendConfig.Docker.Enabled && c.BackendType != backends.BackendTypeMlxLm { return "docker" @@ -203,7 +250,7 @@ func (c *CreateInstanceOptions) GetCommand(backendConfig *config.BackendSettings } // BuildCommandArgs builds command line arguments for the backend -func (c *CreateInstanceOptions) BuildCommandArgs(backendConfig *config.BackendSettings) []string { +func (c *Options) BuildCommandArgs(backendConfig *config.BackendSettings) []string { var args []string @@ -246,7 +293,7 @@ func (c *CreateInstanceOptions) BuildCommandArgs(backendConfig *config.BackendSe return args } -func (c *CreateInstanceOptions) BuildEnvironment(backendConfig *config.BackendSettings) map[string]string { +func (c *Options) BuildEnvironment(backendConfig *config.BackendSettings) map[string]string { env := map[string]string{} if backendConfig.Environment != nil { diff --git a/pkg/instance/timeout_test.go b/pkg/instance/timeout_test.go index c602131..f91a8a8 100644 --- a/pkg/instance/timeout_test.go +++ b/pkg/instance/timeout_test.go @@ -46,7 +46,7 @@ func TestUpdateLastRequestTime(t *testing.T) { LogsDir: "/tmp/test", } - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -77,7 +77,7 @@ func TestShouldTimeout_NotRunning(t *testing.T) { } idleTimeout := 1 // 1 minute - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ IdleTimeout: &idleTimeout, BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -124,7 +124,7 @@ func TestShouldTimeout_NoTimeoutConfigured(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ IdleTimeout: tt.idleTimeout, BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -158,7 +158,7 @@ func TestShouldTimeout_WithinTimeLimit(t *testing.T) { } idleTimeout := 5 // 5 minutes - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ IdleTimeout: &idleTimeout, BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -196,7 +196,7 @@ func TestShouldTimeout_ExceedsTimeLimit(t *testing.T) { } idleTimeout := 1 // 1 minute - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ IdleTimeout: &idleTimeout, BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -252,7 +252,7 @@ func TestTimeoutConfiguration_Validation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ IdleTimeout: tt.inputTimeout, BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index da5a900..a30c084 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -17,9 +17,9 @@ import ( // InstanceManager defines the interface for managing instances of the llama server. type InstanceManager interface { ListInstances() ([]*instance.Instance, error) - CreateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) + CreateInstance(name string, options *instance.Options) (*instance.Instance, error) GetInstance(name string) (*instance.Instance, error) - UpdateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) + UpdateInstance(name string, options *instance.Options) (*instance.Instance, error) DeleteInstance(name string) error StartInstance(name string) (*instance.Instance, error) IsMaxRunningInstancesReached() bool @@ -32,9 +32,9 @@ type InstanceManager interface { type RemoteManager interface { ListRemoteInstances(node *config.NodeConfig) ([]*instance.Instance, error) - CreateRemoteInstance(node *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) + CreateRemoteInstance(node *config.NodeConfig, name string, options *instance.Options) (*instance.Instance, error) GetRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) - UpdateRemoteInstance(node *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) + UpdateRemoteInstance(node *config.NodeConfig, name string, options *instance.Options) (*instance.Instance, error) DeleteRemoteInstance(node *config.NodeConfig, name string) error StartRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) StopRemoteInstance(node *config.NodeConfig, name string) (*instance.Instance, error) diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go index e59e2eb..850c8ca 100644 --- a/pkg/manager/manager_test.go +++ b/pkg/manager/manager_test.go @@ -70,7 +70,7 @@ func TestPersistence(t *testing.T) { // Test instance persistence on creation manager1 := manager.NewInstanceManager(backendConfig, cfg, map[string]config.NodeConfig{}) - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -132,7 +132,7 @@ func TestConcurrentAccess(t *testing.T) { wg.Add(1) go func(index int) { defer wg.Done() - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -169,7 +169,7 @@ func TestShutdown(t *testing.T) { mgr := createTestManager() // Create test instance - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -230,7 +230,7 @@ func TestAutoRestartDisabledInstanceStatus(t *testing.T) { manager1 := manager.NewInstanceManager(backendConfig, cfg, map[string]config.NodeConfig{}) autoRestart := false - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, AutoRestart: &autoRestart, LlamaServerOptions: &llamacpp.LlamaServerOptions{ diff --git a/pkg/manager/operations.go b/pkg/manager/operations.go index cc2dba4..a48ebcb 100644 --- a/pkg/manager/operations.go +++ b/pkg/manager/operations.go @@ -75,7 +75,7 @@ func (im *instanceManager) ListInstances() ([]*instance.Instance, error) { // CreateInstance creates a new instance with the given options and returns it. // The instance is initially in a "stopped" state. -func (im *instanceManager) CreateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { +func (im *instanceManager) CreateInstance(name string, options *instance.Options) (*instance.Instance, error) { if options == nil { return nil, fmt.Errorf("instance options cannot be nil") } @@ -194,7 +194,7 @@ func (im *instanceManager) GetInstance(name string) (*instance.Instance, error) // UpdateInstance updates the options of an existing instance and returns it. // If the instance is running, it will be restarted to apply the new options. -func (im *instanceManager) UpdateInstance(name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { +func (im *instanceManager) UpdateInstance(name string, options *instance.Options) (*instance.Instance, error) { im.mu.RLock() inst, exists := im.instances[name] im.mu.RUnlock() @@ -489,7 +489,7 @@ func (im *instanceManager) GetInstanceLogs(name string, numLines int) (string, e } // getPortFromOptions extracts the port from backend-specific options -func (im *instanceManager) getPortFromOptions(options *instance.CreateInstanceOptions) int { +func (im *instanceManager) getPortFromOptions(options *instance.Options) int { switch options.BackendType { case backends.BackendTypeLlamaCpp: if options.LlamaServerOptions != nil { @@ -508,7 +508,7 @@ func (im *instanceManager) getPortFromOptions(options *instance.CreateInstanceOp } // setPortInOptions sets the port in backend-specific options -func (im *instanceManager) setPortInOptions(options *instance.CreateInstanceOptions, port int) { +func (im *instanceManager) setPortInOptions(options *instance.Options, port int) { switch options.BackendType { case backends.BackendTypeLlamaCpp: if options.LlamaServerOptions != nil { @@ -526,7 +526,7 @@ func (im *instanceManager) setPortInOptions(options *instance.CreateInstanceOpti } // assignAndValidatePort assigns a port if not specified and validates it's not in use -func (im *instanceManager) assignAndValidatePort(options *instance.CreateInstanceOptions) error { +func (im *instanceManager) assignAndValidatePort(options *instance.Options) error { currentPort := im.getPortFromOptions(options) if currentPort == 0 { diff --git a/pkg/manager/operations_test.go b/pkg/manager/operations_test.go index fdeb44f..ff9d68d 100644 --- a/pkg/manager/operations_test.go +++ b/pkg/manager/operations_test.go @@ -13,7 +13,7 @@ import ( func TestCreateInstance_Success(t *testing.T) { manager := createTestManager() - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -40,7 +40,7 @@ func TestCreateInstance_Success(t *testing.T) { func TestCreateInstance_ValidationAndLimits(t *testing.T) { // Test duplicate names mngr := createTestManager() - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -96,7 +96,7 @@ func TestPortManagement(t *testing.T) { manager := createTestManager() // Test auto port assignment - options1 := &instance.CreateInstanceOptions{ + options1 := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -114,7 +114,7 @@ func TestPortManagement(t *testing.T) { } // Test port conflict detection - options2 := &instance.CreateInstanceOptions{ + options2 := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model2.gguf", @@ -132,7 +132,7 @@ func TestPortManagement(t *testing.T) { // Test port release on deletion specificPort := 8080 - options3 := &instance.CreateInstanceOptions{ + options3 := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -160,7 +160,7 @@ func TestPortManagement(t *testing.T) { func TestInstanceOperations(t *testing.T) { manager := createTestManager() - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -183,7 +183,7 @@ func TestInstanceOperations(t *testing.T) { } // Update instance - newOptions := &instance.CreateInstanceOptions{ + newOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/new-model.gguf", diff --git a/pkg/manager/remote_ops.go b/pkg/manager/remote_ops.go index 9e25623..b68e479 100644 --- a/pkg/manager/remote_ops.go +++ b/pkg/manager/remote_ops.go @@ -12,7 +12,7 @@ import ( // stripNodesFromOptions creates a copy of the instance options without the Nodes field // to prevent routing loops when sending requests to remote nodes -func (im *instanceManager) stripNodesFromOptions(options *instance.CreateInstanceOptions) *instance.CreateInstanceOptions { +func (im *instanceManager) stripNodesFromOptions(options *instance.Options) *instance.Options { if options == nil { return nil } @@ -31,7 +31,7 @@ func (im *instanceManager) makeRemoteRequest(nodeConfig *config.NodeConfig, meth var reqBody io.Reader if body != nil { // Strip nodes from CreateInstanceOptions to prevent routing loops - if options, ok := body.(*instance.CreateInstanceOptions); ok { + if options, ok := body.(*instance.Options); ok { body = im.stripNodesFromOptions(options) } @@ -102,7 +102,7 @@ func (im *instanceManager) ListRemoteInstances(nodeConfig *config.NodeConfig) ([ } // CreateRemoteInstance creates a new instance on the remote node -func (im *instanceManager) CreateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { +func (im *instanceManager) CreateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.Options) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/", name) resp, err := im.makeRemoteRequest(nodeConfig, "POST", path, options) @@ -135,7 +135,7 @@ func (im *instanceManager) GetRemoteInstance(nodeConfig *config.NodeConfig, name } // UpdateRemoteInstance updates an existing instance on the remote node -func (im *instanceManager) UpdateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.CreateInstanceOptions) (*instance.Instance, error) { +func (im *instanceManager) UpdateRemoteInstance(nodeConfig *config.NodeConfig, name string, options *instance.Options) (*instance.Instance, error) { path := fmt.Sprintf("/api/v1/instances/%s/", name) resp, err := im.makeRemoteRequest(nodeConfig, "PUT", path, options) diff --git a/pkg/manager/remote_ops_test.go b/pkg/manager/remote_ops_test.go index 94db40b..65fabe6 100644 --- a/pkg/manager/remote_ops_test.go +++ b/pkg/manager/remote_ops_test.go @@ -15,7 +15,7 @@ func TestStripNodesFromOptions(t *testing.T) { } // Test main case: nodes should be stripped, other fields preserved - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, Nodes: []string{"node1", "node2"}, Environment: map[string]string{"TEST": "value"}, diff --git a/pkg/manager/timeout_test.go b/pkg/manager/timeout_test.go index 214a488..7ca3a3e 100644 --- a/pkg/manager/timeout_test.go +++ b/pkg/manager/timeout_test.go @@ -34,7 +34,7 @@ func TestTimeoutFunctionality(t *testing.T) { defer testManager.Shutdown() idleTimeout := 1 // 1 minute - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ IdleTimeout: &idleTimeout, BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -84,7 +84,7 @@ func TestTimeoutFunctionality(t *testing.T) { inst.SetStatus(instance.Stopped) // Test that instance without timeout doesn't timeout - noTimeoutOptions := &instance.CreateInstanceOptions{ + noTimeoutOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -115,21 +115,21 @@ func TestEvictLRUInstance_Success(t *testing.T) { // Don't defer manager.Shutdown() - we'll handle cleanup manually // Create 3 instances with idle timeout enabled (value doesn't matter for LRU logic) - options1 := &instance.CreateInstanceOptions{ + options1 := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model1.gguf", }, IdleTimeout: func() *int { timeout := 1; return &timeout }(), // Any value > 0 } - options2 := &instance.CreateInstanceOptions{ + options2 := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model2.gguf", }, IdleTimeout: func() *int { timeout := 1; return &timeout }(), // Any value > 0 } - options3 := &instance.CreateInstanceOptions{ + options3 := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model3.gguf", @@ -197,7 +197,7 @@ func TestEvictLRUInstance_Success(t *testing.T) { func TestEvictLRUInstance_NoEligibleInstances(t *testing.T) { // Helper function to create instances with different timeout configurations createInstanceWithTimeout := func(manager manager.InstanceManager, name, model string, timeout *int) *instance.Instance { - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: model, diff --git a/pkg/server/handlers_backends.go b/pkg/server/handlers_backends.go index 7d6cab0..6fa833c 100644 --- a/pkg/server/handlers_backends.go +++ b/pkg/server/handlers_backends.go @@ -106,7 +106,7 @@ func (h *Handler) LlamaCppProxy(onDemandStart bool) http.HandlerFunc { // @Accept json // @Produce json // @Param request body ParseCommandRequest true "Command to parse" -// @Success 200 {object} instance.CreateInstanceOptions "Parsed options" +// @Success 200 {object} instance.Options "Parsed options" // @Failure 400 {object} map[string]string "Invalid request or command" // @Failure 500 {object} map[string]string "Internal Server Error" // @Router /backends/llama-cpp/parse-command [post] @@ -135,7 +135,7 @@ func (h *Handler) ParseLlamaCommand() http.HandlerFunc { writeError(w, http.StatusBadRequest, "parse_error", err.Error()) return } - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: llamaOptions, } @@ -154,7 +154,7 @@ func (h *Handler) ParseLlamaCommand() http.HandlerFunc { // @Accept json // @Produce json // @Param request body ParseCommandRequest true "Command to parse" -// @Success 200 {object} instance.CreateInstanceOptions "Parsed options" +// @Success 200 {object} instance.Options "Parsed options" // @Failure 400 {object} map[string]string "Invalid request or command" // @Router /backends/mlx/parse-command [post] func (h *Handler) ParseMlxCommand() http.HandlerFunc { @@ -188,7 +188,7 @@ func (h *Handler) ParseMlxCommand() http.HandlerFunc { // Currently only support mlx_lm backend type backendType := backends.BackendTypeMlxLm - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backendType, MlxServerOptions: mlxOptions, } @@ -208,7 +208,7 @@ func (h *Handler) ParseMlxCommand() http.HandlerFunc { // @Accept json // @Produce json // @Param request body ParseCommandRequest true "Command to parse" -// @Success 200 {object} instance.CreateInstanceOptions "Parsed options" +// @Success 200 {object} instance.Options "Parsed options" // @Failure 400 {object} map[string]string "Invalid request or command" // @Router /backends/vllm/parse-command [post] func (h *Handler) ParseVllmCommand() http.HandlerFunc { @@ -241,7 +241,7 @@ func (h *Handler) ParseVllmCommand() http.HandlerFunc { backendType := backends.BackendTypeVllm - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backendType, VllmServerOptions: vllmOptions, } diff --git a/pkg/server/handlers_instances.go b/pkg/server/handlers_instances.go index 080952c..9cb887e 100644 --- a/pkg/server/handlers_instances.go +++ b/pkg/server/handlers_instances.go @@ -47,7 +47,7 @@ func (h *Handler) ListInstances() http.HandlerFunc { // @Accept json // @Produces json // @Param name path string true "Instance Name" -// @Param options body instance.CreateInstanceOptions true "Instance configuration options" +// @Param options body instance.Options true "Instance configuration options" // @Success 201 {object} instance.Process "Created instance details" // @Failure 400 {string} string "Invalid request body" // @Failure 500 {string} string "Internal Server Error" @@ -60,7 +60,7 @@ func (h *Handler) CreateInstance() http.HandlerFunc { return } - var options instance.CreateInstanceOptions + var options instance.Options if err := json.NewDecoder(r.Body).Decode(&options); err != nil { http.Error(w, "Invalid request body", http.StatusBadRequest) return @@ -122,7 +122,7 @@ func (h *Handler) GetInstance() http.HandlerFunc { // @Accept json // @Produces json // @Param name path string true "Instance Name" -// @Param options body instance.CreateInstanceOptions true "Instance configuration options" +// @Param options body instance.Options true "Instance configuration options" // @Success 200 {object} instance.Process "Updated instance details" // @Failure 400 {string} string "Invalid name format" // @Failure 500 {string} string "Internal Server Error" @@ -135,7 +135,7 @@ func (h *Handler) UpdateInstance() http.HandlerFunc { return } - var options instance.CreateInstanceOptions + var options instance.Options if err := json.NewDecoder(r.Body).Decode(&options); err != nil { http.Error(w, "Invalid request body", http.StatusBadRequest) return diff --git a/pkg/validation/validation.go b/pkg/validation/validation.go index 638e5d2..6d6638d 100644 --- a/pkg/validation/validation.go +++ b/pkg/validation/validation.go @@ -35,7 +35,7 @@ func validateStringForInjection(value string) error { } // ValidateInstanceOptions performs validation based on backend type -func ValidateInstanceOptions(options *instance.CreateInstanceOptions) error { +func ValidateInstanceOptions(options *instance.Options) error { if options == nil { return ValidationError(fmt.Errorf("options cannot be nil")) } @@ -54,7 +54,7 @@ func ValidateInstanceOptions(options *instance.CreateInstanceOptions) error { } // validateLlamaCppOptions validates llama.cpp specific options -func validateLlamaCppOptions(options *instance.CreateInstanceOptions) error { +func validateLlamaCppOptions(options *instance.Options) error { if options.LlamaServerOptions == nil { return ValidationError(fmt.Errorf("llama server options cannot be nil for llama.cpp backend")) } @@ -73,7 +73,7 @@ func validateLlamaCppOptions(options *instance.CreateInstanceOptions) error { } // validateMlxOptions validates MLX backend specific options -func validateMlxOptions(options *instance.CreateInstanceOptions) error { +func validateMlxOptions(options *instance.Options) error { if options.MlxServerOptions == nil { return ValidationError(fmt.Errorf("MLX server options cannot be nil for MLX backend")) } @@ -91,7 +91,7 @@ func validateMlxOptions(options *instance.CreateInstanceOptions) error { } // validateVllmOptions validates vLLM backend specific options -func validateVllmOptions(options *instance.CreateInstanceOptions) error { +func validateVllmOptions(options *instance.Options) error { if options.VllmServerOptions == nil { return ValidationError(fmt.Errorf("vLLM server options cannot be nil for vLLM backend")) } diff --git a/pkg/validation/validation_test.go b/pkg/validation/validation_test.go index 8d8c49e..759ebc3 100644 --- a/pkg/validation/validation_test.go +++ b/pkg/validation/validation_test.go @@ -83,7 +83,7 @@ func TestValidateInstanceOptions_PortValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Port: tt.port, @@ -137,7 +137,7 @@ func TestValidateInstanceOptions_StringInjection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test with Model field (string field) - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: tt.value, @@ -175,7 +175,7 @@ func TestValidateInstanceOptions_ArrayInjection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test with Lora field (array field) - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Lora: tt.array, @@ -194,12 +194,12 @@ func TestValidateInstanceOptions_MultipleFieldInjection(t *testing.T) { // Test that injection in any field is caught tests := []struct { name string - options *instance.CreateInstanceOptions + options *instance.Options wantErr bool }{ { name: "injection in model field", - options: &instance.CreateInstanceOptions{ + options: &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "safe.gguf", @@ -210,7 +210,7 @@ func TestValidateInstanceOptions_MultipleFieldInjection(t *testing.T) { }, { name: "injection in log file", - options: &instance.CreateInstanceOptions{ + options: &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "safe.gguf", @@ -221,7 +221,7 @@ func TestValidateInstanceOptions_MultipleFieldInjection(t *testing.T) { }, { name: "all safe fields", - options: &instance.CreateInstanceOptions{ + options: &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", @@ -247,7 +247,7 @@ func TestValidateInstanceOptions_MultipleFieldInjection(t *testing.T) { func TestValidateInstanceOptions_NonStringFields(t *testing.T) { // Test that non-string fields don't interfere with validation - options := &instance.CreateInstanceOptions{ + options := &instance.Options{ AutoRestart: testutil.BoolPtr(true), MaxRestarts: testutil.IntPtr(5), RestartDelay: testutil.IntPtr(10), From 7bf0809122dd439cdfd232bdb4968de118cba921 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Fri, 17 Oct 2025 00:13:53 +0200 Subject: [PATCH 10/16] Fix test compilation after merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update instance tests to use correct type names: - CreateInstanceOptions -> Options - InstanceStatus -> Status 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- pkg/instance/instance_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index 3fb4226..7413b58 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -207,7 +207,7 @@ func TestSetOptions_PreservesNodes(t *testing.T) { } // Create instance with initial nodes - initialOptions := &instance.CreateInstanceOptions{ + initialOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, Nodes: []string{"worker1"}, LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -216,11 +216,11 @@ func TestSetOptions_PreservesNodes(t *testing.T) { }, } - mockOnStatusChange := func(oldStatus, newStatus instance.InstanceStatus) {} + mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} inst := instance.NewInstance("test-instance", backendConfig, globalSettings, initialOptions, "main", mockOnStatusChange) // Try to update with different nodes - updatedOptions := &instance.CreateInstanceOptions{ + updatedOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, Nodes: []string{"worker2"}, // Attempt to change node LlamaServerOptions: &llamacpp.LlamaServerOptions{ @@ -434,7 +434,7 @@ func TestUnmarshalJSON(t *testing.T) { } } -func TestCreateInstanceOptionsValidation(t *testing.T) { +func TestCreateOptionsValidation(t *testing.T) { tests := []struct { name string maxRestarts *int From 113b51eda246d2882ad00cda8cb2530ceb4a484e Mon Sep 17 00:00:00 2001 From: LordMathis Date: Sat, 18 Oct 2025 00:33:16 +0200 Subject: [PATCH 11/16] Refactor instance node handling to use a map --- pkg/instance/instance.go | 10 ++++++---- pkg/instance/instance_test.go | 8 ++++---- pkg/instance/options.go | 23 ++++++++++++++++++++++- pkg/manager/manager.go | 17 +++++++++++++---- pkg/manager/operations.go | 26 ++++++++++++++------------ pkg/server/handlers_instances.go | 15 ++++++++++++--- pkg/server/handlers_openai.go | 15 ++++++++++++--- 7 files changed, 83 insertions(+), 31 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index da7cda7..1407eba 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -193,8 +193,10 @@ func (i *Instance) GetProxy() (*httputil.ReverseProxy, error) { // Remote instances should not use local proxy - they are handled by RemoteInstanceProxy opts := i.GetOptions() - if opts != nil && len(opts.Nodes) > 0 && opts.Nodes[0] != i.localNodeName { - return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", i.Name) + if opts != nil && len(opts.Nodes) > 0 { + if _, isLocal := opts.Nodes[i.localNodeName]; !isLocal { + return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", i.Name) + } } return i.proxy.GetProxy() @@ -303,8 +305,8 @@ func (i *Instance) IsRemote() bool { return false } - // If the first node is the local node, treat it as a local instance - if opts.Nodes[0] == i.localNodeName { + // If the local node is in the nodes map, treat it as a local instance + if _, isLocal := opts.Nodes[i.localNodeName]; isLocal { return false } diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index 7413b58..5260769 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -209,7 +209,7 @@ func TestSetOptions_PreservesNodes(t *testing.T) { // Create instance with initial nodes initialOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, - Nodes: []string{"worker1"}, + Nodes: map[string]struct{}{"worker1": {}}, LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/model.gguf", Port: 8080, @@ -222,7 +222,7 @@ func TestSetOptions_PreservesNodes(t *testing.T) { // Try to update with different nodes updatedOptions := &instance.Options{ BackendType: backends.BackendTypeLlamaCpp, - Nodes: []string{"worker2"}, // Attempt to change node + Nodes: map[string]struct{}{"worker2": {}}, // Attempt to change node LlamaServerOptions: &llamacpp.LlamaServerOptions{ Model: "/path/to/new-model.gguf", Port: 8081, @@ -233,8 +233,8 @@ func TestSetOptions_PreservesNodes(t *testing.T) { opts := inst.GetOptions() // Nodes should remain unchanged - if len(opts.Nodes) != 1 || opts.Nodes[0] != "worker1" { - t.Errorf("Expected nodes to remain ['worker1'], got %v", opts.Nodes) + if _, exists := opts.Nodes["worker1"]; len(opts.Nodes) != 1 || !exists { + t.Errorf("Expected nodes to contain 'worker1', got %v", opts.Nodes) } // Other options should be updated diff --git a/pkg/instance/options.go b/pkg/instance/options.go index c5db58b..f8af9ee 100644 --- a/pkg/instance/options.go +++ b/pkg/instance/options.go @@ -10,6 +10,7 @@ import ( "llamactl/pkg/config" "log" "maps" + "slices" "sync" ) @@ -29,7 +30,7 @@ type Options struct { BackendType backends.BackendType `json:"backend_type"` BackendOptions map[string]any `json:"backend_options,omitempty"` - Nodes []string `json:"nodes,omitempty"` + Nodes map[string]struct{} `json:"-"` // Backend-specific options LlamaServerOptions *llamacpp.LlamaServerOptions `json:"-"` @@ -87,6 +88,7 @@ func (c *Options) UnmarshalJSON(data []byte) error { // Use anonymous struct to avoid recursion type Alias Options aux := &struct { + Nodes []string `json:"nodes,omitempty"` // Accept JSON array *Alias }{ Alias: (*Alias)(c), @@ -96,6 +98,14 @@ func (c *Options) UnmarshalJSON(data []byte) error { return err } + // Convert nodes array to map + if len(aux.Nodes) > 0 { + c.Nodes = make(map[string]struct{}, len(aux.Nodes)) + for _, node := range aux.Nodes { + c.Nodes[node] = struct{}{} + } + } + // Parse backend-specific options switch c.BackendType { case backends.BackendTypeLlamaCpp: @@ -147,11 +157,22 @@ func (c *Options) MarshalJSON() ([]byte, error) { // Use anonymous struct to avoid recursion type Alias Options aux := struct { + Nodes []string `json:"nodes,omitempty"` // Output as JSON array *Alias }{ Alias: (*Alias)(c), } + // Convert nodes map to array (sorted for consistency) + if len(c.Nodes) > 0 { + aux.Nodes = make([]string, 0, len(c.Nodes)) + for node := range c.Nodes { + aux.Nodes = append(aux.Nodes, node) + } + // Sort for consistent output + slices.Sort(aux.Nodes) + } + // Convert backend-specific options back to BackendOptions map for JSON switch c.BackendType { case backends.BackendTypeLlamaCpp: diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 34ec2e5..b4378a7 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -275,9 +275,19 @@ func (im *instanceManager) loadInstance(name, path string) error { options := persistedInstance.GetOptions() - // Check if this is a remote instance - // An instance is remote if Nodes is specified AND the first node is not the local node - isRemote := options != nil && len(options.Nodes) > 0 && options.Nodes[0] != im.localNodeName + // Check if this is a remote instance (local node not in the Nodes set) + var isRemote bool + var nodeName string + if options != nil { + if _, isLocal := options.Nodes[im.localNodeName]; !isLocal { + // Get the first node from the set + for node := range options.Nodes { + nodeName = node + isRemote = true + break + } + } + } var statusCallback func(oldStatus, newStatus instance.Status) if !isRemote { @@ -296,7 +306,6 @@ func (im *instanceManager) loadInstance(name, path string) error { // Handle remote instance mapping if isRemote { - nodeName := options.Nodes[0] nodeConfig, exists := im.nodeConfigMap[nodeName] if !exists { return fmt.Errorf("node %s not found for remote instance %s", nodeName, name) diff --git a/pkg/manager/operations.go b/pkg/manager/operations.go index 81e374f..5931cfa 100644 --- a/pkg/manager/operations.go +++ b/pkg/manager/operations.go @@ -3,7 +3,6 @@ package manager import ( "fmt" "llamactl/pkg/backends" - "llamactl/pkg/config" "llamactl/pkg/instance" "llamactl/pkg/validation" "os" @@ -27,10 +26,12 @@ func (im *instanceManager) updateLocalInstanceFromRemote(localInst *instance.Ins // Preserve the Nodes field from the local instance localOptions := localInst.GetOptions() - var preservedNodes []string + var preservedNodes map[string]struct{} if localOptions != nil && len(localOptions.Nodes) > 0 { - preservedNodes = make([]string, len(localOptions.Nodes)) - copy(preservedNodes, localOptions.Nodes) + preservedNodes = make(map[string]struct{}, len(localOptions.Nodes)) + for node := range localOptions.Nodes { + preservedNodes[node] = struct{}{} + } } // Create a copy of remote options and restore the Nodes field @@ -98,16 +99,17 @@ func (im *instanceManager) CreateInstance(name string, options *instance.Options return nil, fmt.Errorf("instance with name %s already exists", name) } - // Check if this is a remote instance - // An instance is remote if Nodes is specified AND the first node is not the local node - isRemote := len(options.Nodes) > 0 && options.Nodes[0] != im.localNodeName - var nodeConfig *config.NodeConfig + // Check if this is a remote instance (local node not in the Nodes set) + if _, isLocal := options.Nodes[im.localNodeName]; !isLocal && len(options.Nodes) > 0 { + // Get the first node from the set + var nodeName string + for node := range options.Nodes { + nodeName = node + break + } - if isRemote { // Validate that the node exists - nodeName := options.Nodes[0] // Use first node for now - var exists bool - nodeConfig, exists = im.nodeConfigMap[nodeName] + nodeConfig, exists := im.nodeConfigMap[nodeName] if !exists { return nil, fmt.Errorf("node %s not found", nodeName) } diff --git a/pkg/server/handlers_instances.go b/pkg/server/handlers_instances.go index 9cb887e..9e41190 100644 --- a/pkg/server/handlers_instances.go +++ b/pkg/server/handlers_instances.go @@ -394,12 +394,21 @@ func (h *Handler) ProxyToInstance() http.HandlerFunc { func (h *Handler) RemoteInstanceProxy(w http.ResponseWriter, r *http.Request, name string, inst *instance.Instance) { // Get the node name from instance options options := inst.GetOptions() - if options == nil || len(options.Nodes) == 0 { - http.Error(w, "Instance has no node configured", http.StatusInternalServerError) + if options == nil { + http.Error(w, "Instance has no options configured", http.StatusInternalServerError) return } - nodeName := options.Nodes[0] + // Get the first node from the set + var nodeName string + for node := range options.Nodes { + nodeName = node + break + } + if nodeName == "" { + http.Error(w, "Instance has no node configured", http.StatusInternalServerError) + return + } // Check if we have a cached proxy for this node h.remoteProxiesMu.RLock() diff --git a/pkg/server/handlers_openai.go b/pkg/server/handlers_openai.go index 2849841..075f651 100644 --- a/pkg/server/handlers_openai.go +++ b/pkg/server/handlers_openai.go @@ -155,12 +155,21 @@ func (h *Handler) OpenAIProxy() http.HandlerFunc { func (h *Handler) RemoteOpenAIProxy(w http.ResponseWriter, r *http.Request, modelName string, inst *instance.Instance) { // Get the node name from instance options options := inst.GetOptions() - if options == nil || len(options.Nodes) == 0 { - http.Error(w, "Instance has no node configured", http.StatusInternalServerError) + if options == nil { + http.Error(w, "Instance has no options configured", http.StatusInternalServerError) return } - nodeName := options.Nodes[0] + // Get the first node from the set + var nodeName string + for node := range options.Nodes { + nodeName = node + break + } + if nodeName == "" { + http.Error(w, "Instance has no node configured", http.StatusInternalServerError) + return + } // Check if we have a cached proxy for this node h.remoteProxiesMu.RLock() From 3f834004a82e0bc48c83f93a8f1b9dcbbd3890f1 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Sat, 18 Oct 2025 00:34:18 +0200 Subject: [PATCH 12/16] Rename NewInstance to New --- pkg/instance/instance.go | 6 +++--- pkg/instance/instance_test.go | 14 +++++++------- pkg/instance/timeout_test.go | 12 ++++++------ pkg/manager/manager.go | 2 +- pkg/manager/operations.go | 4 ++-- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 1407eba..5b29737 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -27,7 +27,7 @@ type Instance struct { // Global configuration (read-only, no lock needed) globalInstanceSettings *config.InstancesConfig globalBackendSettings *config.BackendConfig - localNodeName string `json:"-"` // Name of the local node for remote detection + localNodeName string `json:"-"` // Name of the local node for remote detection // Components (can be nil for remote instances or when stopped) logger *logger `json:"-"` // nil for remote instances @@ -47,8 +47,8 @@ type Instance struct { monitorDone chan struct{} `json:"-"` // Channel to signal monitor goroutine completion } -// NewInstance creates a new instance with the given name, log path, and options -func NewInstance(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, opts *Options, localNodeName string, onStatusChange func(oldStatus, newStatus Status)) *Instance { +// New creates a new instance with the given name, log path, and options +func New(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, opts *Options, localNodeName string, onStatusChange func(oldStatus, newStatus Status)) *Instance { // Validate and copy options opts.ValidateAndApplyDefaults(name, globalInstanceSettings) diff --git a/pkg/instance/instance_test.go b/pkg/instance/instance_test.go index 5260769..ac9019e 100644 --- a/pkg/instance/instance_test.go +++ b/pkg/instance/instance_test.go @@ -44,7 +44,7 @@ func TestNewInstance(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) if inst.Name != "test-instance" { t.Errorf("Expected name 'test-instance', got %q", inst.Name) @@ -115,7 +115,7 @@ func TestNewInstance_WithRestartOptions(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - instance := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + instance := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) opts := instance.GetOptions() // Check that explicit values override defaults @@ -164,7 +164,7 @@ func TestSetOptions(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, initialOptions, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, initialOptions, "main", mockOnStatusChange) // Update options newOptions := &instance.Options{ @@ -217,7 +217,7 @@ func TestSetOptions_PreservesNodes(t *testing.T) { } mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, initialOptions, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, initialOptions, "main", mockOnStatusChange) // Try to update with different nodes updatedOptions := &instance.Options{ @@ -274,7 +274,7 @@ func TestGetProxy(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) // Get proxy for the first time proxy1, err := inst.GetProxy() @@ -329,7 +329,7 @@ func TestMarshalJSON(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - instance := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + instance := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) data, err := json.Marshal(instance) if err != nil { @@ -498,7 +498,7 @@ func TestCreateOptionsValidation(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - instance := instance.NewInstance("test", backendConfig, globalSettings, options, "main", mockOnStatusChange) + instance := instance.New("test", backendConfig, globalSettings, options, "main", mockOnStatusChange) opts := instance.GetOptions() if opts.MaxRestarts == nil { diff --git a/pkg/instance/timeout_test.go b/pkg/instance/timeout_test.go index d4f5176..e49e893 100644 --- a/pkg/instance/timeout_test.go +++ b/pkg/instance/timeout_test.go @@ -56,7 +56,7 @@ func TestUpdateLastRequestTime(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) // Test that UpdateLastRequestTime doesn't panic inst.UpdateLastRequestTime() @@ -88,7 +88,7 @@ func TestShouldTimeout_NotRunning(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) // Instance is not running, should not timeout regardless of configuration if inst.ShouldTimeout() { @@ -132,7 +132,7 @@ func TestShouldTimeout_NoTimeoutConfigured(t *testing.T) { }, } - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) // Simulate running state inst.SetStatus(instance.Running) @@ -169,7 +169,7 @@ func TestShouldTimeout_WithinTimeLimit(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) inst.SetStatus(instance.Running) // Update last request time to now @@ -207,7 +207,7 @@ func TestShouldTimeout_ExceedsTimeLimit(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) inst.SetStatus(instance.Running) // Use MockTimeProvider to simulate old last request time @@ -263,7 +263,7 @@ func TestTimeoutConfiguration_Validation(t *testing.T) { // Mock onStatusChange function mockOnStatusChange := func(oldStatus, newStatus instance.Status) {} - inst := instance.NewInstance("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) + inst := instance.New("test-instance", backendConfig, globalSettings, options, "main", mockOnStatusChange) opts := inst.GetOptions() if opts.IdleTimeout == nil || *opts.IdleTimeout != tt.expectedTimeout { diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index b4378a7..894b8df 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -298,7 +298,7 @@ func (im *instanceManager) loadInstance(name, path string) error { } // Create new inst using NewInstance (handles validation, defaults, setup) - inst := instance.NewInstance(name, &im.backendsConfig, &im.instancesConfig, options, im.localNodeName, statusCallback) + inst := instance.New(name, &im.backendsConfig, &im.instancesConfig, options, im.localNodeName, statusCallback) // Restore persisted fields that NewInstance doesn't set inst.Created = persistedInstance.Created diff --git a/pkg/manager/operations.go b/pkg/manager/operations.go index 5931cfa..7129794 100644 --- a/pkg/manager/operations.go +++ b/pkg/manager/operations.go @@ -122,7 +122,7 @@ func (im *instanceManager) CreateInstance(name string, options *instance.Options // Create a local stub that preserves the Nodes field for tracking // We keep the original options (with Nodes) so IsRemote() works correctly - inst := instance.NewInstance(name, &im.backendsConfig, &im.instancesConfig, options, im.localNodeName, nil) + inst := instance.New(name, &im.backendsConfig, &im.instancesConfig, options, im.localNodeName, nil) // Update the local stub with all remote data (preserving Nodes) im.updateLocalInstanceFromRemote(inst, remoteInst) @@ -155,7 +155,7 @@ func (im *instanceManager) CreateInstance(name string, options *instance.Options im.onStatusChange(name, oldStatus, newStatus) } - inst := instance.NewInstance(name, &im.backendsConfig, &im.instancesConfig, options, im.localNodeName, statusCallback) + inst := instance.New(name, &im.backendsConfig, &im.instancesConfig, options, im.localNodeName, statusCallback) im.instances[inst.Name] = inst if err := im.persistInstance(inst); err != nil { From b13f8c471d3997f9c6f10af39a0f9e6ee7bed156 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Sat, 18 Oct 2025 10:28:15 +0200 Subject: [PATCH 13/16] Split off process struct --- pkg/instance/instance.go | 75 +++++-- pkg/instance/lifecycle.go | 406 ---------------------------------- pkg/instance/process.go | 446 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 497 insertions(+), 430 deletions(-) delete mode 100644 pkg/instance/lifecycle.go create mode 100644 pkg/instance/process.go diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 5b29737..5cc0d6e 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -1,16 +1,12 @@ package instance import ( - "context" "encoding/json" "fmt" - "io" "llamactl/pkg/backends" "llamactl/pkg/config" "log" "net/http/httputil" - "os/exec" - "sync" "time" ) @@ -30,21 +26,9 @@ type Instance struct { localNodeName string `json:"-"` // Name of the local node for remote detection // Components (can be nil for remote instances or when stopped) - logger *logger `json:"-"` // nil for remote instances - proxy *proxy `json:"-"` // nil for remote instances - - // internal - cmd *exec.Cmd `json:"-"` // Command to run the instance - ctx context.Context `json:"-"` // Context for managing the instance lifecycle - cancel context.CancelFunc `json:"-"` // Function to cancel the context - stdout io.ReadCloser `json:"-"` // Standard output stream - stderr io.ReadCloser `json:"-"` // Standard error stream - mu sync.RWMutex `json:"-"` // RWMutex for better read/write separation - restarts int `json:"-"` // Number of restarts - - // Restart control - restartCancel context.CancelFunc `json:"-"` // Cancel function for pending restarts - monitorDone chan struct{} `json:"-"` // Channel to signal monitor goroutine completion + process *process `json:"-"` // nil for remote instances, nil when stopped + proxy *proxy `json:"-"` // nil for remote instances, created on demand + logger *logger `json:"-"` // nil for remote instances } // New creates a new instance with the given name, log path, and options @@ -76,6 +60,9 @@ func New(name string, globalBackendSettings *config.BackendConfig, globalInstanc // Create Proxy component instance.proxy = NewProxy(instance) + // Create Process component (will be initialized on first Start) + instance.process = newProcess(instance) + return instance } @@ -204,10 +191,6 @@ func (i *Instance) GetProxy() (*httputil.ReverseProxy, error) { // MarshalJSON implements json.Marshaler for Instance func (i *Instance) MarshalJSON() ([]byte, error) { - // Use read lock since we're only reading data - i.mu.RLock() - defer i.mu.RUnlock() - // Get options opts := i.GetOptions() @@ -280,7 +263,7 @@ func (i *Instance) UnmarshalJSON(data []byte) error { i.options = newOptions(&Options{}) } - // Only create logger and proxy for non-remote instances + // Only create logger, proxy, and process for non-remote instances // Remote instances are metadata only (no logger, proxy, or process) if !i.IsRemote() { if i.logger == nil && i.globalInstanceSettings != nil { @@ -289,6 +272,9 @@ func (i *Instance) UnmarshalJSON(data []byte) error { if i.proxy == nil { i.proxy = NewProxy(i) } + if i.process == nil { + i.process = newProcess(i) + } } return nil @@ -321,6 +307,47 @@ func (i *Instance) GetLogs(num_lines int) (string, error) { return i.logger.GetLogs(num_lines) } +// Start starts the instance, delegating to process component +func (i *Instance) Start() error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be started locally)", i.Name) + } + return i.process.Start() +} + +// Stop stops the instance, delegating to process component +func (i *Instance) Stop() error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be stopped locally)", i.Name) + } + return i.process.Stop() +} + +// Restart restarts the instance, delegating to process component +func (i *Instance) Restart() error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be restarted locally)", i.Name) + } + return i.process.Restart() +} + +// WaitForHealthy waits for the instance to become healthy, delegating to process component +func (i *Instance) WaitForHealthy(timeout int) error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be health checked locally)", i.Name) + } + return i.process.WaitForHealthy(timeout) +} + +// LastRequestTime returns the last request time as a Unix timestamp +// Delegates to the Proxy component +func (i *Instance) LastRequestTime() int64 { + if i.proxy == nil { + return 0 + } + return i.proxy.LastRequestTime() +} + // getBackendHostPort extracts the host and port from instance options // Returns the configured host and port for the backend func (i *Instance) getBackendHostPort() (string, int) { diff --git a/pkg/instance/lifecycle.go b/pkg/instance/lifecycle.go deleted file mode 100644 index 86f73db..0000000 --- a/pkg/instance/lifecycle.go +++ /dev/null @@ -1,406 +0,0 @@ -package instance - -import ( - "context" - "fmt" - "log" - "net/http" - "os" - "os/exec" - "runtime" - "syscall" - "time" - - "llamactl/pkg/backends" - "llamactl/pkg/config" -) - -// Start starts the llama server instance and returns an error if it fails. -func (i *Instance) Start() error { - i.mu.Lock() - defer i.mu.Unlock() - - if i.IsRunning() { - return fmt.Errorf("instance %s is already running", i.Name) - } - - // Safety check: ensure options are valid - if i.options == nil { - return fmt.Errorf("instance %s has no options set", i.Name) - } - - // Reset restart counter when manually starting (not during auto-restart) - // We can detect auto-restart by checking if restartCancel is set - if i.restartCancel == nil { - i.restarts = 0 - } - - // Initialize last request time to current time when starting - if i.proxy != nil { - i.proxy.UpdateLastRequestTime() - } - - // Create context before building command (needed for CommandContext) - i.ctx, i.cancel = context.WithCancel(context.Background()) - - // Create log files - if err := i.logger.Create(); err != nil { - return fmt.Errorf("failed to create log files: %w", err) - } - - // Build command using backend-specific methods - cmd, cmdErr := i.buildCommand() - if cmdErr != nil { - return fmt.Errorf("failed to build command: %w", cmdErr) - } - i.cmd = cmd - - if runtime.GOOS != "windows" { - setProcAttrs(i.cmd) - } - - var err error - i.stdout, err = i.cmd.StdoutPipe() - if err != nil { - i.logger.Close() - return fmt.Errorf("failed to get stdout pipe: %w", err) - } - i.stderr, err = i.cmd.StderrPipe() - if err != nil { - i.stdout.Close() - i.logger.Close() - return fmt.Errorf("failed to get stderr pipe: %w", err) - } - - if err := i.cmd.Start(); err != nil { - return fmt.Errorf("failed to start instance %s: %w", i.Name, err) - } - - i.SetStatus(Running) - - // Create channel for monitor completion signaling - i.monitorDone = make(chan struct{}) - - go i.logger.readOutput(i.stdout) - go i.logger.readOutput(i.stderr) - - go i.monitorProcess() - - return nil -} - -// Stop terminates the subprocess -func (i *Instance) Stop() error { - i.mu.Lock() - - if !i.IsRunning() { - // Even if not running, cancel any pending restart - if i.restartCancel != nil { - i.restartCancel() - i.restartCancel = nil - log.Printf("Cancelled pending restart for instance %s", i.Name) - } - i.mu.Unlock() - return fmt.Errorf("instance %s is not running", i.Name) - } - - // Cancel any pending restart - if i.restartCancel != nil { - i.restartCancel() - i.restartCancel = nil - } - - // Set status to stopped first to signal intentional stop - i.SetStatus(Stopped) - - // Get the monitor done channel before releasing the lock - monitorDone := i.monitorDone - - i.mu.Unlock() - - // Stop the process with SIGINT if cmd exists - if i.cmd != nil && i.cmd.Process != nil { - if err := i.cmd.Process.Signal(syscall.SIGINT); err != nil { - log.Printf("Failed to send SIGINT to instance %s: %v", i.Name, err) - } - } - - // If no process exists, we can return immediately - if i.cmd == nil || monitorDone == nil { - i.logger.Close() - return nil - } - - select { - case <-monitorDone: - // Process exited normally - case <-time.After(30 * time.Second): - // Force kill if it doesn't exit within 30 seconds - if i.cmd != nil && i.cmd.Process != nil { - killErr := i.cmd.Process.Kill() - if killErr != nil { - log.Printf("Failed to force kill instance %s: %v", i.Name, killErr) - } - log.Printf("Instance %s did not stop in time, force killed", i.Name) - - // Wait a bit more for the monitor to finish after force kill - select { - case <-monitorDone: - // Monitor completed after force kill - case <-time.After(2 * time.Second): - log.Printf("Warning: Monitor goroutine did not complete after force kill for instance %s", i.Name) - } - } - } - - i.logger.Close() - - return nil -} - -// LastRequestTime returns the last request time as a Unix timestamp -// Delegates to the Proxy component -func (i *Instance) LastRequestTime() int64 { - if i.proxy == nil { - return 0 - } - return i.proxy.LastRequestTime() -} - -func (i *Instance) WaitForHealthy(timeout int) error { - if !i.IsRunning() { - return fmt.Errorf("instance %s is not running", i.Name) - } - - if timeout <= 0 { - timeout = 30 // Default to 30 seconds if no timeout is specified - } - - ctx, cancel := context.WithTimeout(context.Background(), time.Duration(timeout)*time.Second) - defer cancel() - - // Get host/port from process - host, port := i.getBackendHostPort() - healthURL := fmt.Sprintf("http://%s:%d/health", host, port) - - // Create a dedicated HTTP client for health checks - client := &http.Client{ - Timeout: 5 * time.Second, // 5 second timeout per request - } - - // Helper function to check health directly - checkHealth := func() bool { - req, err := http.NewRequestWithContext(ctx, "GET", healthURL, nil) - if err != nil { - return false - } - - resp, err := client.Do(req) - if err != nil { - return false - } - defer resp.Body.Close() - - return resp.StatusCode == http.StatusOK - } - - // Try immediate check first - if checkHealth() { - return nil // Instance is healthy - } - - // If immediate check failed, start polling - ticker := time.NewTicker(1 * time.Second) - defer ticker.Stop() - - for { - select { - case <-ctx.Done(): - return fmt.Errorf("timeout waiting for instance %s to become healthy after %d seconds", i.Name, timeout) - case <-ticker.C: - if checkHealth() { - return nil // Instance is healthy - } - // Continue polling - } - } -} - -func (i *Instance) monitorProcess() { - defer func() { - i.mu.Lock() - if i.monitorDone != nil { - close(i.monitorDone) - i.monitorDone = nil - } - i.mu.Unlock() - }() - - err := i.cmd.Wait() - - i.mu.Lock() - - // Check if the instance was intentionally stopped - if !i.IsRunning() { - i.mu.Unlock() - return - } - - i.SetStatus(Stopped) - i.logger.Close() - - // Cancel any existing restart context since we're handling a new exit - if i.restartCancel != nil { - i.restartCancel() - i.restartCancel = nil - } - - // Log the exit - if err != nil { - log.Printf("Instance %s crashed with error: %v", i.Name, err) - // Handle restart while holding the lock, then release it - i.handleRestart() - } else { - log.Printf("Instance %s exited cleanly", i.Name) - i.mu.Unlock() - } -} - -// handleRestart manages the restart process while holding the lock -func (i *Instance) handleRestart() { - // Validate restart conditions and get safe parameters - shouldRestart, maxRestarts, restartDelay := i.validateRestartConditions() - if !shouldRestart { - i.SetStatus(Failed) - i.mu.Unlock() - return - } - - i.restarts++ - log.Printf("Auto-restarting instance %s (attempt %d/%d) in %v", - i.Name, i.restarts, maxRestarts, time.Duration(restartDelay)*time.Second) - - // Create a cancellable context for the restart delay - restartCtx, cancel := context.WithCancel(context.Background()) - i.restartCancel = cancel - - // Release the lock before sleeping - i.mu.Unlock() - - // Use context-aware sleep so it can be cancelled - select { - case <-time.After(time.Duration(restartDelay) * time.Second): - // Sleep completed normally, continue with restart - case <-restartCtx.Done(): - // Restart was cancelled - log.Printf("Restart cancelled for instance %s", i.Name) - return - } - - // Restart the instance - if err := i.Start(); err != nil { - log.Printf("Failed to restart instance %s: %v", i.Name, err) - } else { - log.Printf("Successfully restarted instance %s", i.Name) - // Clear the cancel function - i.mu.Lock() - i.restartCancel = nil - i.mu.Unlock() - } -} - -// validateRestartConditions checks if the instance should be restarted and returns the parameters -func (i *Instance) validateRestartConditions() (shouldRestart bool, maxRestarts int, restartDelay int) { - opts := i.GetOptions() - if opts == nil { - log.Printf("Instance %s not restarting: options are nil", i.Name) - return false, 0, 0 - } - - if opts.AutoRestart == nil || !*opts.AutoRestart { - log.Printf("Instance %s not restarting: AutoRestart is disabled", i.Name) - return false, 0, 0 - } - - if opts.MaxRestarts == nil { - log.Printf("Instance %s not restarting: MaxRestarts is nil", i.Name) - return false, 0, 0 - } - - if opts.RestartDelay == nil { - log.Printf("Instance %s not restarting: RestartDelay is nil", i.Name) - return false, 0, 0 - } - - // Values are already validated during unmarshaling/SetOptions - maxRestarts = *opts.MaxRestarts - restartDelay = *opts.RestartDelay - - if i.restarts >= maxRestarts { - log.Printf("Instance %s exceeded max restart attempts (%d)", i.Name, maxRestarts) - return false, 0, 0 - } - - return true, maxRestarts, restartDelay -} - -// buildCommand builds the command to execute using backend-specific logic -func (i *Instance) buildCommand() (*exec.Cmd, error) { - // Get options - opts := i.GetOptions() - if opts == nil { - return nil, fmt.Errorf("instance options are nil") - } - - // Get backend configuration - backendConfig, err := i.getBackendConfig() - if err != nil { - return nil, err - } - - // Build the environment variables - env := opts.BuildEnvironment(backendConfig) - - // Get the command to execute - command := opts.GetCommand(backendConfig) - - // Build command arguments - args := opts.BuildCommandArgs(backendConfig) - - // Create the exec.Cmd - cmd := exec.CommandContext(i.ctx, command, args...) - - // Start with host environment variables - cmd.Env = os.Environ() - - // Add/override with backend-specific environment variables - for k, v := range env { - cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v)) - } - - return cmd, nil -} - -// getBackendConfig resolves the backend configuration for the current instance -func (i *Instance) getBackendConfig() (*config.BackendSettings, error) { - opts := i.GetOptions() - if opts == nil { - return nil, fmt.Errorf("instance options are nil") - } - - var backendTypeStr string - - switch opts.BackendType { - case backends.BackendTypeLlamaCpp: - backendTypeStr = "llama-cpp" - case backends.BackendTypeMlxLm: - backendTypeStr = "mlx" - case backends.BackendTypeVllm: - backendTypeStr = "vllm" - default: - return nil, fmt.Errorf("unsupported backend type: %s", opts.BackendType) - } - - settings := i.globalBackendSettings.GetBackendSettings(backendTypeStr) - return &settings, nil -} diff --git a/pkg/instance/process.go b/pkg/instance/process.go new file mode 100644 index 0000000..afbe0e8 --- /dev/null +++ b/pkg/instance/process.go @@ -0,0 +1,446 @@ +package instance + +import ( + "context" + "fmt" + "io" + "log" + "net/http" + "os" + "os/exec" + "runtime" + "sync" + "syscall" + "time" + + "llamactl/pkg/backends" + "llamactl/pkg/config" +) + +// process manages the OS process lifecycle for a local instance (unexported). +// process owns its complete lifecycle including auto-restart logic. +type process struct { + instance *Instance // Back-reference for SetStatus, GetOptions + + mu sync.RWMutex + cmd *exec.Cmd + ctx context.Context + cancel context.CancelFunc + stdout io.ReadCloser + stderr io.ReadCloser + restarts int // process owns restart counter + restartCancel context.CancelFunc + monitorDone chan struct{} +} + +// newProcess creates a new process component for the given instance +func newProcess(instance *Instance) *process { + return &process{ + instance: instance, + } +} + +// Start starts the OS process and returns an error if it fails. +func (p *process) Start() error { + p.mu.Lock() + defer p.mu.Unlock() + + if p.instance.IsRunning() { + return fmt.Errorf("instance %s is already running", p.instance.Name) + } + + // Safety check: ensure options are valid + if p.instance.options == nil { + return fmt.Errorf("instance %s has no options set", p.instance.Name) + } + + // Reset restart counter when manually starting (not during auto-restart) + // We can detect auto-restart by checking if restartCancel is set + if p.restartCancel == nil { + p.restarts = 0 + } + + // Initialize last request time to current time when starting + if p.instance.proxy != nil { + p.instance.proxy.UpdateLastRequestTime() + } + + // Create context before building command (needed for CommandContext) + p.ctx, p.cancel = context.WithCancel(context.Background()) + + // Create log files + if err := p.instance.logger.Create(); err != nil { + return fmt.Errorf("failed to create log files: %w", err) + } + + // Build command using backend-specific methods + cmd, cmdErr := p.buildCommand() + if cmdErr != nil { + return fmt.Errorf("failed to build command: %w", cmdErr) + } + p.cmd = cmd + + if runtime.GOOS != "windows" { + setProcAttrs(p.cmd) + } + + var err error + p.stdout, err = p.cmd.StdoutPipe() + if err != nil { + p.instance.logger.Close() + return fmt.Errorf("failed to get stdout pipe: %w", err) + } + p.stderr, err = p.cmd.StderrPipe() + if err != nil { + p.stdout.Close() + p.instance.logger.Close() + return fmt.Errorf("failed to get stderr pipe: %w", err) + } + + if err := p.cmd.Start(); err != nil { + return fmt.Errorf("failed to start instance %s: %w", p.instance.Name, err) + } + + p.instance.SetStatus(Running) + + // Create channel for monitor completion signaling + p.monitorDone = make(chan struct{}) + + go p.instance.logger.readOutput(p.stdout) + go p.instance.logger.readOutput(p.stderr) + + go p.monitorProcess() + + return nil +} + +// Stop terminates the subprocess without restarting +func (p *process) Stop() error { + p.mu.Lock() + + if !p.instance.IsRunning() { + // Even if not running, cancel any pending restart + if p.restartCancel != nil { + p.restartCancel() + p.restartCancel = nil + log.Printf("Cancelled pending restart for instance %s", p.instance.Name) + } + p.mu.Unlock() + return fmt.Errorf("instance %s is not running", p.instance.Name) + } + + // Cancel any pending restart + if p.restartCancel != nil { + p.restartCancel() + p.restartCancel = nil + } + + // Set status to stopped first to signal intentional stop + p.instance.SetStatus(Stopped) + + // Get the monitor done channel before releasing the lock + monitorDone := p.monitorDone + + p.mu.Unlock() + + // Stop the process with SIGINT if cmd exists + if p.cmd != nil && p.cmd.Process != nil { + if err := p.cmd.Process.Signal(syscall.SIGINT); err != nil { + log.Printf("Failed to send SIGINT to instance %s: %v", p.instance.Name, err) + } + } + + // If no process exists, we can return immediately + if p.cmd == nil || monitorDone == nil { + p.instance.logger.Close() + return nil + } + + select { + case <-monitorDone: + // Process exited normally + case <-time.After(30 * time.Second): + // Force kill if it doesn't exit within 30 seconds + if p.cmd != nil && p.cmd.Process != nil { + killErr := p.cmd.Process.Kill() + if killErr != nil { + log.Printf("Failed to force kill instance %s: %v", p.instance.Name, killErr) + } + log.Printf("Instance %s did not stop in time, force killed", p.instance.Name) + + // Wait a bit more for the monitor to finish after force kill + select { + case <-monitorDone: + // Monitor completed after force kill + case <-time.After(2 * time.Second): + log.Printf("Warning: Monitor goroutine did not complete after force kill for instance %s", p.instance.Name) + } + } + } + + p.instance.logger.Close() + + return nil +} + +// Restart manually restarts the process (resets restart counter) +func (p *process) Restart() error { + // Stop the process first + if err := p.Stop(); err != nil { + // If it's not running, that's ok - we'll just start it + if err.Error() != fmt.Sprintf("instance %s is not running", p.instance.Name) { + return fmt.Errorf("failed to stop instance during restart: %w", err) + } + } + + // Reset restart counter for manual restart + p.mu.Lock() + p.restarts = 0 + p.mu.Unlock() + + // Start the process + return p.Start() +} + +// WaitForHealthy waits for the process to become healthy +func (p *process) WaitForHealthy(timeout int) error { + if !p.instance.IsRunning() { + return fmt.Errorf("instance %s is not running", p.instance.Name) + } + + if timeout <= 0 { + timeout = 30 // Default to 30 seconds if no timeout is specified + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(timeout)*time.Second) + defer cancel() + + // Get host/port from instance + host, port := p.instance.getBackendHostPort() + healthURL := fmt.Sprintf("http://%s:%d/health", host, port) + + // Create a dedicated HTTP client for health checks + client := &http.Client{ + Timeout: 5 * time.Second, // 5 second timeout per request + } + + // Helper function to check health directly + checkHealth := func() bool { + req, err := http.NewRequestWithContext(ctx, "GET", healthURL, nil) + if err != nil { + return false + } + + resp, err := client.Do(req) + if err != nil { + return false + } + defer resp.Body.Close() + + return resp.StatusCode == http.StatusOK + } + + // Try immediate check first + if checkHealth() { + return nil // Instance is healthy + } + + // If immediate check failed, start polling + ticker := time.NewTicker(1 * time.Second) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return fmt.Errorf("timeout waiting for instance %s to become healthy after %d seconds", p.instance.Name, timeout) + case <-ticker.C: + if checkHealth() { + return nil // Instance is healthy + } + // Continue polling + } + } +} + +// monitorProcess monitors the OS process and handles crashes/exits +func (p *process) monitorProcess() { + defer func() { + p.mu.Lock() + if p.monitorDone != nil { + close(p.monitorDone) + p.monitorDone = nil + } + p.mu.Unlock() + }() + + err := p.cmd.Wait() + + p.mu.Lock() + + // Check if the instance was intentionally stopped + if !p.instance.IsRunning() { + p.mu.Unlock() + return + } + + p.instance.SetStatus(Stopped) + p.instance.logger.Close() + + // Cancel any existing restart context since we're handling a new exit + if p.restartCancel != nil { + p.restartCancel() + p.restartCancel = nil + } + + // Log the exit + if err != nil { + log.Printf("Instance %s crashed with error: %v", p.instance.Name, err) + // Handle auto-restart logic + p.handleAutoRestart(err) + } else { + log.Printf("Instance %s exited cleanly", p.instance.Name) + p.mu.Unlock() + } +} + +// shouldAutoRestart checks if the process should auto-restart +func (p *process) shouldAutoRestart() bool { + opts := p.instance.GetOptions() + if opts == nil { + log.Printf("Instance %s not restarting: options are nil", p.instance.Name) + return false + } + + if opts.AutoRestart == nil || !*opts.AutoRestart { + log.Printf("Instance %s not restarting: AutoRestart is disabled", p.instance.Name) + return false + } + + if opts.MaxRestarts == nil { + log.Printf("Instance %s not restarting: MaxRestarts is nil", p.instance.Name) + return false + } + + maxRestarts := *opts.MaxRestarts + if p.restarts >= maxRestarts { + log.Printf("Instance %s exceeded max restart attempts (%d)", p.instance.Name, maxRestarts) + return false + } + + return true +} + +// handleAutoRestart manages the auto-restart process +func (p *process) handleAutoRestart(err error) { + // Check if should restart + if !p.shouldAutoRestart() { + p.instance.SetStatus(Failed) + p.mu.Unlock() + return + } + + // Get restart parameters + opts := p.instance.GetOptions() + if opts.RestartDelay == nil { + log.Printf("Instance %s not restarting: RestartDelay is nil", p.instance.Name) + p.instance.SetStatus(Failed) + p.mu.Unlock() + return + } + + restartDelay := *opts.RestartDelay + maxRestarts := *opts.MaxRestarts + + p.restarts++ + log.Printf("Auto-restarting instance %s (attempt %d/%d) in %v", + p.instance.Name, p.restarts, maxRestarts, time.Duration(restartDelay)*time.Second) + + // Create a cancellable context for the restart delay + restartCtx, cancel := context.WithCancel(context.Background()) + p.restartCancel = cancel + + // Release the lock before sleeping + p.mu.Unlock() + + // Use context-aware sleep so it can be cancelled + select { + case <-time.After(time.Duration(restartDelay) * time.Second): + // Sleep completed normally, continue with restart + case <-restartCtx.Done(): + // Restart was cancelled + log.Printf("Restart cancelled for instance %s", p.instance.Name) + return + } + + // Restart the instance + if err := p.Start(); err != nil { + log.Printf("Failed to restart instance %s: %v", p.instance.Name, err) + } else { + log.Printf("Successfully restarted instance %s", p.instance.Name) + // Clear the cancel function + p.mu.Lock() + p.restartCancel = nil + p.mu.Unlock() + } +} + +// buildCommand builds the command to execute using backend-specific logic +func (p *process) buildCommand() (*exec.Cmd, error) { + // Get options + opts := p.instance.GetOptions() + if opts == nil { + return nil, fmt.Errorf("instance options are nil") + } + + // Get backend configuration + backendConfig, err := p.getBackendConfig() + if err != nil { + return nil, err + } + + // Build the environment variables + env := opts.BuildEnvironment(backendConfig) + + // Get the command to execute + command := opts.GetCommand(backendConfig) + + // Build command arguments + args := opts.BuildCommandArgs(backendConfig) + + // Create the exec.Cmd + cmd := exec.CommandContext(p.ctx, command, args...) + + // Start with host environment variables + cmd.Env = os.Environ() + + // Add/override with backend-specific environment variables + for k, v := range env { + cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v)) + } + + return cmd, nil +} + +// getBackendConfig resolves the backend configuration for the current instance +func (p *process) getBackendConfig() (*config.BackendSettings, error) { + opts := p.instance.GetOptions() + if opts == nil { + return nil, fmt.Errorf("instance options are nil") + } + + var backendTypeStr string + + switch opts.BackendType { + case backends.BackendTypeLlamaCpp: + backendTypeStr = "llama-cpp" + case backends.BackendTypeMlxLm: + backendTypeStr = "mlx" + case backends.BackendTypeVllm: + backendTypeStr = "vllm" + default: + return nil, fmt.Errorf("unsupported backend type: %s", opts.BackendType) + } + + settings := p.instance.globalBackendSettings.GetBackendSettings(backendTypeStr) + return &settings, nil +} From a7740000d2635ad8bcab7fbbbccc18c8a6e15d60 Mon Sep 17 00:00:00 2001 From: LordMathis Date: Sat, 18 Oct 2025 10:39:04 +0200 Subject: [PATCH 14/16] Refactor instance creation to initialize logger, proxy, and process only for local instances --- pkg/instance/instance.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 5cc0d6e..cc80bf0 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -36,9 +36,6 @@ func New(name string, globalBackendSettings *config.BackendConfig, globalInstanc // Validate and copy options opts.ValidateAndApplyDefaults(name, globalInstanceSettings) - // Create the instance logger - logger := NewLogger(name, globalInstanceSettings.LogsDir) - // Create status wrapper status := newStatus(Stopped) status.onStatusChange = onStatusChange @@ -52,16 +49,17 @@ func New(name string, globalBackendSettings *config.BackendConfig, globalInstanc globalInstanceSettings: globalInstanceSettings, globalBackendSettings: globalBackendSettings, localNodeName: localNodeName, - logger: logger, Created: time.Now().Unix(), status: status, } - // Create Proxy component - instance.proxy = NewProxy(instance) - - // Create Process component (will be initialized on first Start) - instance.process = newProcess(instance) + // Only create logger, proxy, and process for local instances + // Remote instances are metadata only (no logger, proxy, or process) + if !instance.IsRemote() { + instance.logger = NewLogger(name, globalInstanceSettings.LogsDir) + instance.proxy = NewProxy(instance) + instance.process = newProcess(instance) + } return instance } From 8ac4b370c9d82e0d37bcba54819068f6be3e5fbb Mon Sep 17 00:00:00 2001 From: LordMathis Date: Sat, 18 Oct 2025 11:25:26 +0200 Subject: [PATCH 15/16] Unexport struct methods --- pkg/instance/instance.go | 293 ++++++++++++++++++++------------------- pkg/instance/logger.go | 14 +- pkg/instance/options.go | 22 +-- pkg/instance/process.go | 46 +++--- pkg/instance/proxy.go | 59 ++++---- pkg/instance/status.go | 12 +- pkg/instance/timeout.go | 18 --- 7 files changed, 228 insertions(+), 236 deletions(-) delete mode 100644 pkg/instance/timeout.go diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index cc80bf0..dd14743 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -12,29 +12,27 @@ import ( // Instance represents a running instance of the llama server type Instance struct { - // Immutable identity (no locking needed after creation) Name string `json:"name"` Created int64 `json:"created,omitempty"` // Unix timestamp when the instance was created - // Mutable state - each owns its own lock - status *status `json:"-"` // unexported - status owns its lock - options *options `json:"-"` // unexported - options owns its lock - - // Global configuration (read-only, no lock needed) + // Global configuration globalInstanceSettings *config.InstancesConfig globalBackendSettings *config.BackendConfig localNodeName string `json:"-"` // Name of the local node for remote detection - // Components (can be nil for remote instances or when stopped) - process *process `json:"-"` // nil for remote instances, nil when stopped - proxy *proxy `json:"-"` // nil for remote instances, created on demand - logger *logger `json:"-"` // nil for remote instances + status *status `json:"-"` + options *options `json:"-"` + + // Components (can be nil for remote instances) + process *process `json:"-"` + proxy *proxy `json:"-"` + logger *logger `json:"-"` } -// New creates a new instance with the given name, log path, and options +// New creates a new instance with the given name, log path, options and local node name func New(name string, globalBackendSettings *config.BackendConfig, globalInstanceSettings *config.InstancesConfig, opts *Options, localNodeName string, onStatusChange func(oldStatus, newStatus Status)) *Instance { // Validate and copy options - opts.ValidateAndApplyDefaults(name, globalInstanceSettings) + opts.validateAndApplyDefaults(name, globalInstanceSettings) // Create status wrapper status := newStatus(Stopped) @@ -54,45 +52,76 @@ func New(name string, globalBackendSettings *config.BackendConfig, globalInstanc } // Only create logger, proxy, and process for local instances - // Remote instances are metadata only (no logger, proxy, or process) if !instance.IsRemote() { - instance.logger = NewLogger(name, globalInstanceSettings.LogsDir) - instance.proxy = NewProxy(instance) + instance.logger = newLogger(name, globalInstanceSettings.LogsDir) + instance.proxy = newProxy(instance) instance.process = newProcess(instance) } return instance } -// GetOptions returns the current options, delegating to options component +// Start starts the instance +func (i *Instance) Start() error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be started locally)", i.Name) + } + return i.process.start() +} + +// Stop stops the instance +func (i *Instance) Stop() error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be stopped locally)", i.Name) + } + return i.process.stop() +} + +// Restart restarts the instance +func (i *Instance) Restart() error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be restarted locally)", i.Name) + } + return i.process.restart() +} + +// WaitForHealthy waits for the instance to become healthy +func (i *Instance) WaitForHealthy(timeout int) error { + if i.process == nil { + return fmt.Errorf("instance %s has no process component (remote instances cannot be health checked locally)", i.Name) + } + return i.process.waitForHealthy(timeout) +} + +// GetOptions returns the current options func (i *Instance) GetOptions() *Options { if i.options == nil { return nil } - return i.options.Get() + return i.options.get() } -// GetStatus returns the current status, delegating to status component +// GetStatus returns the current status func (i *Instance) GetStatus() Status { if i.status == nil { return Stopped } - return i.status.Get() + return i.status.get() } -// SetStatus sets the status, delegating to status component +// SetStatus sets the status func (i *Instance) SetStatus(s Status) { if i.status != nil { - i.status.Set(s) + i.status.set(s) } } -// IsRunning returns true if the status is Running, delegating to status component +// IsRunning returns true if the status is Running func (i *Instance) IsRunning() bool { if i.status == nil { return false } - return i.status.IsRunning() + return i.status.isRunning() } func (i *Instance) GetPort() int { @@ -137,7 +166,7 @@ func (i *Instance) GetHost() string { return "" } -// SetOptions sets the options, delegating to options component +// SetOptions sets the options func (i *Instance) SetOptions(opts *Options) { if opts == nil { log.Println("Warning: Attempted to set nil options on instance", i.Name) @@ -145,32 +174,31 @@ 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 { - opts.Nodes = i.options.Get().Nodes + if i.options != nil && i.options.get() != nil && i.options.get().Nodes != nil { + opts.Nodes = i.options.get().Nodes } // Validate and copy options - opts.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) + opts.validateAndApplyDefaults(i.Name, i.globalInstanceSettings) if i.options != nil { - i.options.Set(opts) + i.options.set(opts) } // Clear the proxy so it gets recreated with new options if i.proxy != nil { - i.proxy.clearProxy() + i.proxy.clear() } } // SetTimeProvider sets a custom time provider for testing -// Delegates to the Proxy component func (i *Instance) SetTimeProvider(tp TimeProvider) { if i.proxy != nil { - i.proxy.SetTimeProvider(tp) + i.proxy.setTimeProvider(tp) } } -// GetProxy returns the reverse proxy for this instance, delegating to Proxy component +// GetProxy returns the reverse proxy for this instance func (i *Instance) GetProxy() (*httputil.ReverseProxy, error) { if i.proxy == nil { return nil, fmt.Errorf("instance %s has no proxy component", i.Name) @@ -184,7 +212,93 @@ func (i *Instance) GetProxy() (*httputil.ReverseProxy, error) { } } - return i.proxy.GetProxy() + return i.proxy.get() +} + +func (i *Instance) IsRemote() bool { + opts := i.GetOptions() + if opts == nil { + return false + } + + // If no nodes specified, it's a local instance + if len(opts.Nodes) == 0 { + return false + } + + // If the local node is in the nodes map, treat it as a local instance + if _, isLocal := opts.Nodes[i.localNodeName]; isLocal { + return false + } + + // Otherwise, it's a remote instance + return true +} + +// GetLogs retrieves the last n lines of logs from the instance +func (i *Instance) GetLogs(num_lines int) (string, error) { + if i.logger == nil { + return "", fmt.Errorf("instance %s has no logger (remote instances don't have logs)", i.Name) + } + return i.logger.getLogs(num_lines) +} + +// LastRequestTime returns the last request time as a Unix timestamp +func (i *Instance) LastRequestTime() int64 { + if i.proxy == nil { + return 0 + } + return i.proxy.getLastRequestTime() +} + +// UpdateLastRequestTime updates the last request access time for the instance via proxy +func (i *Instance) UpdateLastRequestTime() { + if i.proxy != nil { + i.proxy.updateLastRequestTime() + } +} + +// ShouldTimeout checks if the instance should timeout based on idle time +func (i *Instance) ShouldTimeout() bool { + if i.proxy == nil { + return false + } + return i.proxy.shouldTimeout() +} + +// getBackendHostPort extracts the host and port from instance options +// Returns the configured host and port for the backend +func (i *Instance) getBackendHostPort() (string, int) { + opts := i.GetOptions() + if opts == nil { + return "localhost", 0 + } + + var host string + var port int + switch opts.BackendType { + case backends.BackendTypeLlamaCpp: + if opts.LlamaServerOptions != nil { + host = opts.LlamaServerOptions.Host + port = opts.LlamaServerOptions.Port + } + case backends.BackendTypeMlxLm: + if opts.MlxServerOptions != nil { + host = opts.MlxServerOptions.Host + port = opts.MlxServerOptions.Port + } + case backends.BackendTypeVllm: + if opts.VllmServerOptions != nil { + host = opts.VllmServerOptions.Host + port = opts.VllmServerOptions.Port + } + } + + if host == "" { + host = "localhost" + } + + return host, port } // MarshalJSON implements json.Marshaler for Instance @@ -209,7 +323,6 @@ func (i *Instance) MarshalJSON() ([]byte, error) { } } - // Explicitly serialize to maintain backward compatible JSON format return json.Marshal(&struct { Name string `json:"name"` Status *status `json:"status"` @@ -247,9 +360,9 @@ func (i *Instance) UnmarshalJSON(data []byte) error { // Handle options with validation and defaults if i.options != nil { - opts := i.options.Get() + opts := i.options.get() if opts != nil { - opts.ValidateAndApplyDefaults(i.Name, i.globalInstanceSettings) + opts.validateAndApplyDefaults(i.Name, i.globalInstanceSettings) } } @@ -262,13 +375,12 @@ func (i *Instance) UnmarshalJSON(data []byte) error { } // Only create logger, proxy, and process for non-remote instances - // Remote instances are metadata only (no logger, proxy, or process) if !i.IsRemote() { if i.logger == nil && i.globalInstanceSettings != nil { - i.logger = NewLogger(i.Name, i.globalInstanceSettings.LogsDir) + i.logger = newLogger(i.Name, i.globalInstanceSettings.LogsDir) } if i.proxy == nil { - i.proxy = NewProxy(i) + i.proxy = newProxy(i) } if i.process == nil { i.process = newProcess(i) @@ -277,106 +389,3 @@ func (i *Instance) UnmarshalJSON(data []byte) error { return nil } - -func (i *Instance) IsRemote() bool { - opts := i.GetOptions() - if opts == nil { - return false - } - - // If no nodes specified, it's a local instance - if len(opts.Nodes) == 0 { - return false - } - - // If the local node is in the nodes map, treat it as a local instance - if _, isLocal := opts.Nodes[i.localNodeName]; isLocal { - return false - } - - // Otherwise, it's a remote instance - return true -} - -func (i *Instance) GetLogs(num_lines int) (string, error) { - if i.logger == nil { - return "", fmt.Errorf("instance %s has no logger (remote instances don't have logs)", i.Name) - } - return i.logger.GetLogs(num_lines) -} - -// Start starts the instance, delegating to process component -func (i *Instance) Start() error { - if i.process == nil { - return fmt.Errorf("instance %s has no process component (remote instances cannot be started locally)", i.Name) - } - return i.process.Start() -} - -// Stop stops the instance, delegating to process component -func (i *Instance) Stop() error { - if i.process == nil { - return fmt.Errorf("instance %s has no process component (remote instances cannot be stopped locally)", i.Name) - } - return i.process.Stop() -} - -// Restart restarts the instance, delegating to process component -func (i *Instance) Restart() error { - if i.process == nil { - return fmt.Errorf("instance %s has no process component (remote instances cannot be restarted locally)", i.Name) - } - return i.process.Restart() -} - -// WaitForHealthy waits for the instance to become healthy, delegating to process component -func (i *Instance) WaitForHealthy(timeout int) error { - if i.process == nil { - return fmt.Errorf("instance %s has no process component (remote instances cannot be health checked locally)", i.Name) - } - return i.process.WaitForHealthy(timeout) -} - -// LastRequestTime returns the last request time as a Unix timestamp -// Delegates to the Proxy component -func (i *Instance) LastRequestTime() int64 { - if i.proxy == nil { - return 0 - } - return i.proxy.LastRequestTime() -} - -// getBackendHostPort extracts the host and port from instance options -// Returns the configured host and port for the backend -func (i *Instance) getBackendHostPort() (string, int) { - opts := i.GetOptions() - if opts == nil { - return "localhost", 0 - } - - var host string - var port int - switch opts.BackendType { - case backends.BackendTypeLlamaCpp: - if opts.LlamaServerOptions != nil { - host = opts.LlamaServerOptions.Host - port = opts.LlamaServerOptions.Port - } - case backends.BackendTypeMlxLm: - if opts.MlxServerOptions != nil { - host = opts.MlxServerOptions.Host - port = opts.MlxServerOptions.Port - } - case backends.BackendTypeVllm: - if opts.VllmServerOptions != nil { - host = opts.VllmServerOptions.Host - port = opts.VllmServerOptions.Port - } - } - - if host == "" { - host = "localhost" - } - - return host, port -} diff --git a/pkg/instance/logger.go b/pkg/instance/logger.go index 014ca33..f836411 100644 --- a/pkg/instance/logger.go +++ b/pkg/instance/logger.go @@ -18,15 +18,15 @@ type logger struct { mu sync.RWMutex } -func NewLogger(name string, logDir string) *logger { +func newLogger(name string, logDir string) *logger { return &logger{ name: name, logDir: logDir, } } -// Create creates and opens the log files for stdout and stderr -func (i *logger) Create() error { +// create creates and opens the log files for stdout and stderr +func (i *logger) create() error { i.mu.Lock() defer i.mu.Unlock() @@ -56,8 +56,8 @@ func (i *logger) Create() error { return nil } -// GetLogs retrieves the last n lines of logs from the instance -func (i *logger) GetLogs(num_lines int) (string, error) { +// getLogs retrieves the last n lines of logs from the instance +func (i *logger) getLogs(num_lines int) (string, error) { i.mu.RLock() defer i.mu.RUnlock() @@ -97,8 +97,8 @@ func (i *logger) GetLogs(num_lines int) (string, error) { return strings.Join(lines[start:], "\n"), nil } -// closeLogFile closes the log files -func (i *logger) Close() { +// close closes the log files +func (i *logger) close() { i.mu.Lock() defer i.mu.Unlock() diff --git a/pkg/instance/options.go b/pkg/instance/options.go index f8af9ee..1dddb15 100644 --- a/pkg/instance/options.go +++ b/pkg/instance/options.go @@ -51,15 +51,15 @@ func newOptions(opts *Options) *options { } } -// Get returns a copy of the current options -func (o *options) Get() *Options { +// get returns a copy of the current options +func (o *options) get() *Options { o.mu.RLock() defer o.mu.RUnlock() return o.opts } -// Set updates the options -func (o *options) Set(opts *Options) { +// set updates the options +func (o *options) set(opts *Options) { o.mu.Lock() defer o.mu.Unlock() o.opts = opts @@ -222,8 +222,8 @@ func (c *Options) MarshalJSON() ([]byte, error) { return json.Marshal(aux) } -// ValidateAndApplyDefaults validates the instance options and applies constraints -func (c *Options) ValidateAndApplyDefaults(name string, globalSettings *config.InstancesConfig) { +// validateAndApplyDefaults validates the instance options and applies constraints +func (c *Options) validateAndApplyDefaults(name string, globalSettings *config.InstancesConfig) { // Validate and apply constraints if c.MaxRestarts != nil && *c.MaxRestarts < 0 { log.Printf("Instance %s MaxRestarts value (%d) cannot be negative, setting to 0", name, *c.MaxRestarts) @@ -261,7 +261,8 @@ func (c *Options) ValidateAndApplyDefaults(name string, globalSettings *config.I } } -func (c *Options) GetCommand(backendConfig *config.BackendSettings) string { +// getCommand builds the command to run the backend +func (c *Options) getCommand(backendConfig *config.BackendSettings) string { if backendConfig.Docker != nil && backendConfig.Docker.Enabled && c.BackendType != backends.BackendTypeMlxLm { return "docker" @@ -270,8 +271,8 @@ func (c *Options) GetCommand(backendConfig *config.BackendSettings) string { return backendConfig.Command } -// BuildCommandArgs builds command line arguments for the backend -func (c *Options) BuildCommandArgs(backendConfig *config.BackendSettings) []string { +// buildCommandArgs builds command line arguments for the backend +func (c *Options) buildCommandArgs(backendConfig *config.BackendSettings) []string { var args []string @@ -314,7 +315,8 @@ func (c *Options) BuildCommandArgs(backendConfig *config.BackendSettings) []stri return args } -func (c *Options) BuildEnvironment(backendConfig *config.BackendSettings) map[string]string { +// buildEnvironment builds the environment variables for the backend process +func (c *Options) buildEnvironment(backendConfig *config.BackendSettings) map[string]string { env := map[string]string{} if backendConfig.Environment != nil { diff --git a/pkg/instance/process.go b/pkg/instance/process.go index afbe0e8..9c7cfec 100644 --- a/pkg/instance/process.go +++ b/pkg/instance/process.go @@ -17,7 +17,7 @@ import ( "llamactl/pkg/config" ) -// process manages the OS process lifecycle for a local instance (unexported). +// process manages the OS process lifecycle for a local instance. // process owns its complete lifecycle including auto-restart logic. type process struct { instance *Instance // Back-reference for SetStatus, GetOptions @@ -28,7 +28,7 @@ type process struct { cancel context.CancelFunc stdout io.ReadCloser stderr io.ReadCloser - restarts int // process owns restart counter + restarts int restartCancel context.CancelFunc monitorDone chan struct{} } @@ -40,8 +40,8 @@ func newProcess(instance *Instance) *process { } } -// Start starts the OS process and returns an error if it fails. -func (p *process) Start() error { +// start starts the OS process and returns an error if it fails. +func (p *process) start() error { p.mu.Lock() defer p.mu.Unlock() @@ -62,14 +62,14 @@ func (p *process) Start() error { // Initialize last request time to current time when starting if p.instance.proxy != nil { - p.instance.proxy.UpdateLastRequestTime() + p.instance.proxy.updateLastRequestTime() } // Create context before building command (needed for CommandContext) p.ctx, p.cancel = context.WithCancel(context.Background()) // Create log files - if err := p.instance.logger.Create(); err != nil { + if err := p.instance.logger.create(); err != nil { return fmt.Errorf("failed to create log files: %w", err) } @@ -87,13 +87,13 @@ func (p *process) Start() error { var err error p.stdout, err = p.cmd.StdoutPipe() if err != nil { - p.instance.logger.Close() + p.instance.logger.close() return fmt.Errorf("failed to get stdout pipe: %w", err) } p.stderr, err = p.cmd.StderrPipe() if err != nil { p.stdout.Close() - p.instance.logger.Close() + p.instance.logger.close() return fmt.Errorf("failed to get stderr pipe: %w", err) } @@ -114,8 +114,8 @@ func (p *process) Start() error { return nil } -// Stop terminates the subprocess without restarting -func (p *process) Stop() error { +// stop terminates the subprocess without restarting +func (p *process) stop() error { p.mu.Lock() if !p.instance.IsRunning() { @@ -152,7 +152,7 @@ func (p *process) Stop() error { // If no process exists, we can return immediately if p.cmd == nil || monitorDone == nil { - p.instance.logger.Close() + p.instance.logger.close() return nil } @@ -178,15 +178,15 @@ func (p *process) Stop() error { } } - p.instance.logger.Close() + p.instance.logger.close() return nil } -// Restart manually restarts the process (resets restart counter) -func (p *process) Restart() error { +// restart manually restarts the process (resets restart counter) +func (p *process) restart() error { // Stop the process first - if err := p.Stop(); err != nil { + if err := p.stop(); err != nil { // If it's not running, that's ok - we'll just start it if err.Error() != fmt.Sprintf("instance %s is not running", p.instance.Name) { return fmt.Errorf("failed to stop instance during restart: %w", err) @@ -199,11 +199,11 @@ func (p *process) Restart() error { p.mu.Unlock() // Start the process - return p.Start() + return p.start() } -// WaitForHealthy waits for the process to become healthy -func (p *process) WaitForHealthy(timeout int) error { +// waitForHealthy waits for the process to become healthy +func (p *process) waitForHealthy(timeout int) error { if !p.instance.IsRunning() { return fmt.Errorf("instance %s is not running", p.instance.Name) } @@ -284,7 +284,7 @@ func (p *process) monitorProcess() { } p.instance.SetStatus(Stopped) - p.instance.logger.Close() + p.instance.logger.close() // Cancel any existing restart context since we're handling a new exit if p.restartCancel != nil { @@ -373,7 +373,7 @@ func (p *process) handleAutoRestart(err error) { } // Restart the instance - if err := p.Start(); err != nil { + if err := p.start(); err != nil { log.Printf("Failed to restart instance %s: %v", p.instance.Name, err) } else { log.Printf("Successfully restarted instance %s", p.instance.Name) @@ -399,13 +399,13 @@ func (p *process) buildCommand() (*exec.Cmd, error) { } // Build the environment variables - env := opts.BuildEnvironment(backendConfig) + env := opts.buildEnvironment(backendConfig) // Get the command to execute - command := opts.GetCommand(backendConfig) + command := opts.getCommand(backendConfig) // Build command arguments - args := opts.BuildCommandArgs(backendConfig) + args := opts.buildCommandArgs(backendConfig) // Create the exec.Cmd cmd := exec.CommandContext(p.ctx, command, args...) diff --git a/pkg/instance/proxy.go b/pkg/instance/proxy.go index 1a515bd..321095b 100644 --- a/pkg/instance/proxy.go +++ b/pkg/instance/proxy.go @@ -25,7 +25,7 @@ func (realTimeProvider) Now() time.Time { // proxy manages HTTP reverse proxy and request tracking for an instance. type proxy struct { - process *Instance // Owner reference - Proxy is owned by Process + instance *Instance mu sync.RWMutex proxy *httputil.ReverseProxy @@ -35,44 +35,44 @@ type proxy struct { timeProvider TimeProvider } -// NewProxy creates a new Proxy for the given process -func NewProxy(process *Instance) *proxy { +// newProxy creates a new Proxy for the given instance +func newProxy(instance *Instance) *proxy { return &proxy{ - process: process, + instance: instance, timeProvider: realTimeProvider{}, } } -// GetProxy returns the reverse proxy for this instance, creating it if needed. +// get returns the reverse proxy for this instance, creating it if needed. // Uses sync.Once to ensure thread-safe one-time initialization. -func (p *proxy) GetProxy() (*httputil.ReverseProxy, error) { +func (p *proxy) get() (*httputil.ReverseProxy, error) { // sync.Once guarantees buildProxy() is called exactly once // Other callers block until first initialization completes p.proxyOnce.Do(func() { - p.proxy, p.proxyErr = p.buildProxy() + p.proxy, p.proxyErr = p.build() }) return p.proxy, p.proxyErr } -// buildProxy creates the reverse proxy based on instance options -func (p *proxy) buildProxy() (*httputil.ReverseProxy, error) { - options := p.process.GetOptions() +// build creates the reverse proxy based on instance options +func (p *proxy) build() (*httputil.ReverseProxy, error) { + options := p.instance.GetOptions() if options == nil { - return nil, fmt.Errorf("instance %s has no options set", p.process.Name) + return nil, fmt.Errorf("instance %s has no options set", p.instance.Name) } // Remote instances should not use local proxy - they are handled by RemoteInstanceProxy if len(options.Nodes) > 0 { - return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", p.process.Name) + return nil, fmt.Errorf("instance %s is a remote instance and should not use local proxy", p.instance.Name) } // Get host/port from process - host, port := p.process.getBackendHostPort() + host, port := p.instance.getBackendHostPort() targetURL, err := url.Parse(fmt.Sprintf("http://%s:%d", host, port)) if err != nil { - return nil, fmt.Errorf("failed to parse target URL for instance %s: %w", p.process.Name, err) + return nil, fmt.Errorf("failed to parse target URL for instance %s: %w", p.instance.Name, err) } proxy := httputil.NewSingleHostReverseProxy(targetURL) @@ -81,11 +81,11 @@ func (p *proxy) buildProxy() (*httputil.ReverseProxy, error) { var responseHeaders map[string]string switch options.BackendType { case backends.BackendTypeLlamaCpp: - responseHeaders = p.process.globalBackendSettings.LlamaCpp.ResponseHeaders + responseHeaders = p.instance.globalBackendSettings.LlamaCpp.ResponseHeaders case backends.BackendTypeVllm: - responseHeaders = p.process.globalBackendSettings.VLLM.ResponseHeaders + responseHeaders = p.instance.globalBackendSettings.VLLM.ResponseHeaders case backends.BackendTypeMlxLm: - responseHeaders = p.process.globalBackendSettings.MLX.ResponseHeaders + responseHeaders = p.instance.globalBackendSettings.MLX.ResponseHeaders } proxy.ModifyResponse = func(resp *http.Response) error { @@ -107,9 +107,8 @@ func (p *proxy) buildProxy() (*httputil.ReverseProxy, error) { return proxy, nil } -// clearProxy resets the proxy, allowing it to be recreated when options change. -// This resets the sync.Once so the next GetProxy call will rebuild the proxy. -func (p *proxy) clearProxy() { +// clear resets the proxy, allowing it to be recreated when options change. +func (p *proxy) clear() { p.mu.Lock() defer p.mu.Unlock() @@ -118,24 +117,24 @@ func (p *proxy) clearProxy() { p.proxyOnce = sync.Once{} // Reset Once for next GetProxy call } -// UpdateLastRequestTime updates the last request access time for the instance -func (p *proxy) UpdateLastRequestTime() { +// updateLastRequestTime updates the last request access time for the instance +func (p *proxy) updateLastRequestTime() { lastRequestTime := p.timeProvider.Now().Unix() p.lastRequestTime.Store(lastRequestTime) } -// LastRequestTime returns the last request time as a Unix timestamp -func (p *proxy) LastRequestTime() int64 { +// getLastRequestTime returns the last request time as a Unix timestamp +func (p *proxy) getLastRequestTime() int64 { return p.lastRequestTime.Load() } -// ShouldTimeout checks if the instance should timeout based on idle time -func (p *proxy) ShouldTimeout() bool { - if !p.process.IsRunning() { +// shouldTimeout checks if the instance should timeout based on idle time +func (p *proxy) shouldTimeout() bool { + if !p.instance.IsRunning() { return false } - options := p.process.GetOptions() + options := p.instance.GetOptions() if options == nil || options.IdleTimeout == nil || *options.IdleTimeout <= 0 { return false } @@ -150,7 +149,7 @@ func (p *proxy) ShouldTimeout() bool { return (p.timeProvider.Now().Unix() - lastRequest) > idleTimeoutSeconds } -// SetTimeProvider sets a custom time provider for testing -func (p *proxy) SetTimeProvider(tp TimeProvider) { +// setTimeProvider sets a custom time provider for testing +func (p *proxy) setTimeProvider(tp TimeProvider) { p.timeProvider = tp } diff --git a/pkg/instance/status.go b/pkg/instance/status.go index 8c24f49..92e8669 100644 --- a/pkg/instance/status.go +++ b/pkg/instance/status.go @@ -69,15 +69,15 @@ func newStatus(initial Status) *status { } } -// Get returns the current status -func (st *status) Get() Status { +// get returns the current status +func (st *status) get() Status { st.mu.RLock() defer st.mu.RUnlock() return st.s } -// Set updates the status and triggers the onStatusChange callback if set -func (st *status) Set(newStatus Status) { +// set updates the status and triggers the onStatusChange callback if set +func (st *status) set(newStatus Status) { st.mu.Lock() oldStatus := st.s st.s = newStatus @@ -90,8 +90,8 @@ func (st *status) Set(newStatus Status) { } } -// IsRunning returns true if the status is Running -func (st *status) IsRunning() bool { +// isRunning returns true if the status is Running +func (st *status) isRunning() bool { st.mu.RLock() defer st.mu.RUnlock() return st.s == Running diff --git a/pkg/instance/timeout.go b/pkg/instance/timeout.go deleted file mode 100644 index 93e2477..0000000 --- a/pkg/instance/timeout.go +++ /dev/null @@ -1,18 +0,0 @@ -package instance - -// UpdateLastRequestTime updates the last request access time for the instance via proxy -// Delegates to the Proxy component -func (i *Instance) UpdateLastRequestTime() { - if i.proxy != nil { - i.proxy.UpdateLastRequestTime() - } -} - -// ShouldTimeout checks if the instance should timeout based on idle time -// Delegates to the Proxy component -func (i *Instance) ShouldTimeout() bool { - if i.proxy == nil { - return false - } - return i.proxy.ShouldTimeout() -} From 851c73f0584cb5aa33354cb8cf5013e728dee4ad Mon Sep 17 00:00:00 2001 From: LordMathis Date: Sat, 18 Oct 2025 13:19:01 +0200 Subject: [PATCH 16/16] Add tests for status change callback and options preservation --- pkg/instance/instance.go | 2 +- pkg/instance/instance_test.go | 301 ++++++++++++++++++++++++++++++++++ pkg/instance/timeout_test.go | 274 ------------------------------- 3 files changed, 302 insertions(+), 275 deletions(-) delete mode 100644 pkg/instance/timeout_test.go 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) - } - }) - } -}