Ensure that file statuses reflect proxy and runtime status (#1380)

With the file-based statuses, we have the potential to go out of sync with the containers and proxy processes. Every time we read the file, if the file's status is "running", then check the status of the workload in the runtime, and check the proxy process. If there is disagreement, set the status in the file to "unhealthy".

We may want to tweak this behaviour in future, for example, either deleting the unhealthy workloads, or perhaps trying to fix them. This is intended as a first step, since toolhive currently does not do a good job of detecting these problems.

Author: dmjb

pkg/workloads/statuses/file_status.go

@@ -14,7 +14,9 @@ import (
 
 	rt "github.com/stacklok/toolhive/pkg/container/runtime"
 	"github.com/stacklok/toolhive/pkg/core"
+	"github.com/stacklok/toolhive/pkg/labels"
 	"github.com/stacklok/toolhive/pkg/logger"
+	"github.com/stacklok/toolhive/pkg/transport/proxy"
 	"github.com/stacklok/toolhive/pkg/workloads/types"
 )
 
@@ -92,22 +94,9 @@ func (f *fileStatusManager) GetWorkload(ctx context.Context, workloadName string
 		return core.Workload{}, err
 	}
 
-	// If file was found and workload is running, get additional info from runtime
+	// If file was found and workload is running, validate against runtime
 	if fileFound && result.Status == rt.WorkloadStatusRunning {
-		// TODO: Find discrepancies between the file and runtime workload.
-		runtimeResult, err := f.getWorkloadFromRuntime(ctx, workloadName)
-		if err != nil {
-			return core.Workload{}, err
-		}
-		// Use runtime data but preserve file-based status info
-		fileStatus := result.Status
-		fileStatusContext := result.StatusContext
-		fileCreatedAt := result.CreatedAt
-		result = runtimeResult
-		result.Status = fileStatus               // Keep the file status
-		result.StatusContext = fileStatusContext // Keep the file status context
-		result.CreatedAt = fileCreatedAt         // Keep the file created time
-		return result, nil
+		return f.validateRunningWorkload(ctx, workloadName, result)
 	}
 
 	// If file was found and workload is not running, return file data
@@ -421,3 +410,99 @@ func (f *fileStatusManager) getWorkloadsFromFiles() (map[string]core.Workload, e
 
 	return workloads, nil
 }
+
+// validateRunningWorkload validates that a workload marked as running in the file
+// is actually running in the runtime and has a healthy proxy process if applicable.
+func (f *fileStatusManager) validateRunningWorkload(
+	ctx context.Context, workloadName string, result core.Workload,
+) (core.Workload, error) {
+	// Get raw container info from runtime (before label filtering)
+	containerInfo, err := f.runtime.GetWorkloadInfo(ctx, workloadName)
+	if err != nil {
+		return core.Workload{}, err
+	}
+
+	// Check if runtime status matches file status
+	if containerInfo.State != rt.WorkloadStatusRunning {
+		return f.handleRuntimeMismatch(ctx, workloadName, result, containerInfo)
+	}
+
+	// Check if proxy process is running when workload is running
+	if unhealthyWorkload, isUnhealthy := f.checkProxyHealth(ctx, workloadName, result, containerInfo); isUnhealthy {
+		return unhealthyWorkload, nil
+	}
+
+	// Runtime and proxy confirm workload is healthy - merge runtime data with file status
+	return f.mergeHealthyWorkloadData(containerInfo, result)
+}
+
+// handleRuntimeMismatch handles the case where file indicates running but runtime shows different status
+func (f *fileStatusManager) handleRuntimeMismatch(
+	ctx context.Context, workloadName string, result core.Workload, containerInfo rt.ContainerInfo,
+) (core.Workload, error) {
+	contextMsg := fmt.Sprintf("workload status mismatch: file indicates running, but runtime shows %s", containerInfo.State)
+	if err := f.SetWorkloadStatus(ctx, workloadName, rt.WorkloadStatusUnhealthy, contextMsg); err != nil {
+		logger.Warnf("failed to update workload %s status to unhealthy: %v", workloadName, err)
+	}
+
+	// Convert to workload and return unhealthy status
+	runtimeResult, err := types.WorkloadFromContainerInfo(&containerInfo)
+	if err != nil {
+		return core.Workload{}, err
+	}
+
+	runtimeResult.Status = rt.WorkloadStatusUnhealthy
+	runtimeResult.StatusContext = contextMsg
+	runtimeResult.CreatedAt = result.CreatedAt // Keep the original file created time
+	return runtimeResult, nil
+}
+
+// checkProxyHealth checks if the proxy process is running for the workload.
+// Returns (unhealthyWorkload, true) if proxy is not running, (emptyWorkload, false) if proxy is healthy or not applicable.
+func (f *fileStatusManager) checkProxyHealth(
+	ctx context.Context, workloadName string, result core.Workload, containerInfo rt.ContainerInfo,
+) (core.Workload, bool) {
+	// Use original container labels (before filtering) to get base name
+	baseName := labels.GetContainerBaseName(containerInfo.Labels)
+	if baseName == "" {
+		return core.Workload{}, false // No proxy check needed
+	}
+
+	proxyRunning := proxy.IsRunning(baseName)
+	if proxyRunning {
+		return core.Workload{}, false // Proxy is healthy
+	}
+
+	// Proxy is not running, but workload should be running
+	contextMsg := fmt.Sprintf("proxy process not running: workload shows running but proxy process for %s is not active",
+		baseName)
+	if err := f.SetWorkloadStatus(ctx, workloadName, rt.WorkloadStatusUnhealthy, contextMsg); err != nil {
+		logger.Warnf("failed to update workload %s status to unhealthy: %v", workloadName, err)
+	}
+
+	// Convert to workload and return unhealthy status
+	runtimeResult, err := types.WorkloadFromContainerInfo(&containerInfo)
+	if err != nil {
+		logger.Warnf("failed to convert container info for unhealthy workload %s: %v", workloadName, err)
+		return core.Workload{}, false // Return false to avoid double error handling
+	}
+
+	runtimeResult.Status = rt.WorkloadStatusUnhealthy
+	runtimeResult.StatusContext = contextMsg
+	runtimeResult.CreatedAt = result.CreatedAt // Keep the original file created time
+	return runtimeResult, true
+}
+
+// mergeHealthyWorkloadData merges runtime container data with file-based status information
+func (*fileStatusManager) mergeHealthyWorkloadData(containerInfo rt.ContainerInfo, result core.Workload) (core.Workload, error) {
+	// Runtime and proxy confirm workload is healthy - use runtime data but preserve file-based status info
+	runtimeResult, err := types.WorkloadFromContainerInfo(&containerInfo)
+	if err != nil {
+		return core.Workload{}, err
+	}
+
+	runtimeResult.Status = result.Status               // Keep the file status (running)
+	runtimeResult.StatusContext = result.StatusContext // Keep the file status context
+	runtimeResult.CreatedAt = result.CreatedAt         // Keep the file created time
+	return runtimeResult, nil
+}

pkg/workloads/statuses/file_status_test.go

@@ -622,3 +622,241 @@ func TestFileStatusManager_ListWorkloads(t *testing.T) {
 		})
 	}
 }
+
+func TestFileStatusManager_GetWorkload_UnhealthyDetection(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	tempDir := t.TempDir()
+	mockRuntime := mocks.NewMockRuntime(ctrl)
+	manager := &fileStatusManager{
+		baseDir: tempDir,
+		runtime: mockRuntime,
+	}
+	ctx := context.Background()
+
+	// First, set the workload status to running in the file
+	err := manager.SetWorkloadStatus(ctx, "test-workload", rt.WorkloadStatusRunning, "container started")
+	require.NoError(t, err)
+
+	// Mock the runtime to return a stopped workload (mismatch with file)
+	stoppedInfo := rt.ContainerInfo{
+		Name:    "test-workload",
+		Image:   "test-image:latest",
+		Status:  "Exited (0) 2 minutes ago",
+		State:   rt.WorkloadStatusStopped, // Runtime says stopped
+		Created: time.Now().Add(-10 * time.Minute),
+		Labels: map[string]string{
+			"toolhive":      "true",
+			"toolhive-name": "test-workload",
+		},
+	}
+
+	mockRuntime.EXPECT().
+		GetWorkloadInfo(gomock.Any(), "test-workload").
+		Return(stoppedInfo, nil)
+
+	// Mock the call to SetWorkloadStatus that will be made to update to unhealthy
+	// This is tricky because we need to intercept the call but allow it to proceed
+	// For simplicity, we'll just allow the call to succeed
+	mockRuntime.EXPECT().
+		GetWorkloadInfo(gomock.Any(), "test-workload").
+		Return(stoppedInfo, nil).
+		AnyTimes() // Allow multiple calls during the SetWorkloadStatus operation
+
+	// Get the workload - this should detect the mismatch and return unhealthy status
+	workload, err := manager.GetWorkload(ctx, "test-workload")
+	require.NoError(t, err)
+
+	// Verify the workload is marked as unhealthy
+	assert.Equal(t, "test-workload", workload.Name)
+	assert.Equal(t, rt.WorkloadStatusUnhealthy, workload.Status)
+	assert.Contains(t, workload.StatusContext, "workload status mismatch")
+	assert.Contains(t, workload.StatusContext, "file indicates running")
+	assert.Contains(t, workload.StatusContext, "runtime shows stopped")
+	assert.Equal(t, "test-image:latest", workload.Package)
+
+	// Verify the file was updated to unhealthy status
+	// Get the workload again (this time without runtime mismatch since status is now unhealthy)
+	statusFilePath := filepath.Join(tempDir, "test-workload.json")
+	data, err := os.ReadFile(statusFilePath)
+	require.NoError(t, err)
+
+	var statusFile workloadStatusFile
+	err = json.Unmarshal(data, &statusFile)
+	require.NoError(t, err)
+
+	assert.Equal(t, rt.WorkloadStatusUnhealthy, statusFile.Status)
+	assert.Contains(t, statusFile.StatusContext, "workload status mismatch")
+}
+
+func TestFileStatusManager_GetWorkload_HealthyRunningWorkload(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	tempDir := t.TempDir()
+	mockRuntime := mocks.NewMockRuntime(ctrl)
+	manager := &fileStatusManager{
+		baseDir: tempDir,
+		runtime: mockRuntime,
+	}
+	ctx := context.Background()
+
+	// Set the workload status to running in the file
+	err := manager.SetWorkloadStatus(ctx, "healthy-workload", rt.WorkloadStatusRunning, "container started")
+	require.NoError(t, err)
+
+	// Mock the runtime to return a running workload (matches file)
+	runningInfo := rt.ContainerInfo{
+		Name:    "healthy-workload",
+		Image:   "test-image:latest",
+		Status:  "Up 5 minutes",
+		State:   rt.WorkloadStatusRunning, // Runtime says running (matches file)
+		Created: time.Now().Add(-10 * time.Minute),
+		Labels: map[string]string{
+			"toolhive":      "true",
+			"toolhive-name": "healthy-workload",
+		},
+	}
+
+	mockRuntime.EXPECT().
+		GetWorkloadInfo(gomock.Any(), "healthy-workload").
+		Return(runningInfo, nil)
+
+	// Get the workload - this should remain running since file and runtime match
+	workload, err := manager.GetWorkload(ctx, "healthy-workload")
+	require.NoError(t, err)
+
+	// Verify the workload remains running
+	assert.Equal(t, "healthy-workload", workload.Name)
+	assert.Equal(t, rt.WorkloadStatusRunning, workload.Status)
+	assert.Equal(t, "container started", workload.StatusContext) // Original file context preserved
+	assert.Equal(t, "test-image:latest", workload.Package)
+}
+
+func TestFileStatusManager_GetWorkload_ProxyNotRunning(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	tempDir := t.TempDir()
+	mockRuntime := mocks.NewMockRuntime(ctrl)
+
+	// Create file status manager directly instead of using NewFileStatusManager
+	manager := &fileStatusManager{
+		baseDir: tempDir,
+		runtime: mockRuntime,
+	}
+	ctx := context.Background()
+
+	// First, create a status file manually to ensure file is found
+	statusFile := workloadStatusFile{
+		Status:        rt.WorkloadStatusRunning,
+		StatusContext: "container started",
+		CreatedAt:     time.Now(),
+		UpdatedAt:     time.Now(),
+	}
+	statusFilePath := filepath.Join(tempDir, "proxy-down-workload.json")
+	statusData, err := json.Marshal(statusFile)
+	require.NoError(t, err)
+	err = os.WriteFile(statusFilePath, statusData, 0644)
+	require.NoError(t, err)
+
+	// Mock the runtime to return a running workload with proper labels
+	runningInfo := rt.ContainerInfo{
+		Name:    "proxy-down-workload",
+		Image:   "test-image:latest",
+		Status:  "Up 5 minutes",
+		State:   rt.WorkloadStatusRunning, // Runtime says running (matches file)
+		Created: time.Now().Add(-10 * time.Minute),
+		Labels: map[string]string{
+			"toolhive":          "true",
+			"toolhive-name":     "proxy-down-workload",
+			"toolhive-basename": "proxy-down-workload", // This is the base name for proxy
+		},
+	}
+
+	// Mock the GetWorkloadInfo call that will be made during the proxy check
+	mockRuntime.EXPECT().
+		GetWorkloadInfo(gomock.Any(), "proxy-down-workload").
+		Return(runningInfo, nil).
+		AnyTimes() // Allow multiple calls during the SetWorkloadStatus operation as well
+
+	// Note: proxy.IsRunning will check the actual system, but since there's no proxy
+	// process running for "proxy-down-workload", it will return false
+
+	// Get the workload - this should detect the proxy is not running and return unhealthy
+	workload, err := manager.GetWorkload(ctx, "proxy-down-workload")
+	require.NoError(t, err)
+
+	// Verify the workload is marked as unhealthy due to proxy not running
+	assert.Equal(t, "proxy-down-workload", workload.Name)
+	assert.Equal(t, rt.WorkloadStatusUnhealthy, workload.Status)
+	assert.Contains(t, workload.StatusContext, "proxy process not running")
+	assert.Contains(t, workload.StatusContext, "proxy-down-workload")
+	assert.Contains(t, workload.StatusContext, "not active")
+	assert.Equal(t, "test-image:latest", workload.Package)
+
+	// Verify the file was updated to unhealthy status
+	data, err := os.ReadFile(statusFilePath)
+	require.NoError(t, err)
+
+	var updatedStatusFile workloadStatusFile
+	err = json.Unmarshal(data, &updatedStatusFile)
+	require.NoError(t, err)
+
+	assert.Equal(t, rt.WorkloadStatusUnhealthy, updatedStatusFile.Status)
+	assert.Contains(t, updatedStatusFile.StatusContext, "proxy process not running")
+}
+
+func TestFileStatusManager_GetWorkload_HealthyWithProxy(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	tempDir := t.TempDir()
+	mockRuntime := mocks.NewMockRuntime(ctrl)
+	manager := &fileStatusManager{
+		baseDir: tempDir,
+		runtime: mockRuntime,
+	}
+	ctx := context.Background()
+
+	// Set the workload status to running in the file
+	err := manager.SetWorkloadStatus(ctx, "healthy-with-proxy", rt.WorkloadStatusRunning, "container started")
+	require.NoError(t, err)
+
+	// Mock the runtime to return a running workload without base name (no proxy check)
+	runningInfo := rt.ContainerInfo{
+		Name:    "healthy-with-proxy",
+		Image:   "test-image:latest",
+		Status:  "Up 5 minutes",
+		State:   rt.WorkloadStatusRunning,
+		Created: time.Now().Add(-10 * time.Minute),
+		Labels: map[string]string{
+			"toolhive":      "true",
+			"toolhive-name": "healthy-with-proxy",
+			// No toolhive-base-name label, so proxy check will be skipped
+		},
+	}
+
+	mockRuntime.EXPECT().
+		GetWorkloadInfo(gomock.Any(), "healthy-with-proxy").
+		Return(runningInfo, nil)
+
+	// Get the workload - this should remain running since there's no base name for proxy check
+	workload, err := manager.GetWorkload(ctx, "healthy-with-proxy")
+	require.NoError(t, err)
+
+	// Verify the workload remains running (no proxy check due to missing base name)
+	assert.Equal(t, "healthy-with-proxy", workload.Name)
+	assert.Equal(t, rt.WorkloadStatusRunning, workload.Status)
+	assert.Equal(t, "container started", workload.StatusContext) // Original file context preserved
+	assert.Equal(t, "test-image:latest", workload.Package)
+}