Drop any code which writes to PID files (#2899)

The PID of the proxy process is now stored in the status file. The PID
file code was left for backwards compatibility, and we can now safely
remove it.

Future PRs will drop the code which reads/deletes/migrates the PID
files.

Author: dmjb

pkg/process/pid.go

@@ -70,45 +70,6 @@ func getPIDFilePathWithFallback(containerBaseName string) (string, error) {
 	return newPath, nil
 }
 
-// WritePIDFile writes a process ID to a file
-// For full version compatibility, it writes to both the new XDG location and the old temp location
-func WritePIDFile(containerBaseName string, pid int) error {
-	// Skip PID file operations in Kubernetes runtime
-	if runtime.IsKubernetesRuntime() {
-		return nil
-	}
-
-	pidContent := []byte(fmt.Sprintf("%d", pid))
-
-	// Write to the new XDG location first
-	newPath, err := getPIDFilePath(containerBaseName)
-	if err != nil {
-		return fmt.Errorf("failed to get PID file path: %v", err)
-	}
-
-	// Ensure the directory exists before writing
-	if err := os.MkdirAll(filepath.Dir(newPath), 0750); err != nil {
-		return fmt.Errorf("failed to create PID directory: %v", err)
-	}
-
-	if err := os.WriteFile(newPath, pidContent, 0600); err != nil {
-		return fmt.Errorf("failed to write PID file: %v", err)
-	}
-
-	// Also write to the old temp location for backward/forward compatibility
-	// This is best-effort - don't fail the operation
-	oldPath := getOldPIDFilePath(containerBaseName)
-
-	_ = os.WriteFile(oldPath, pidContent, 0600)
-
-	return nil
-}
-
-// WriteCurrentPIDFile writes the current process ID to a file
-func WriteCurrentPIDFile(containerBaseName string) error {
-	return WritePIDFile(containerBaseName, os.Getpid())
-}
-
 // ReadPIDFile reads the process ID from a file
 // It checks both the new XDG location and the old temp directory location
 // Note: containerBaseName is pre-sanitized by the caller

pkg/process/pid_test.go

@@ -83,46 +83,6 @@ func TestPIDFileBackwardCompatibility(t *testing.T) {
 		assert.Equal(t, newPID, pid, "Should read from new location when both exist")
 	})
 
-	t.Run("WritePIDFile_WritesBothLocations", func(t *testing.T) {
-		//nolint:paralleltest // File system operations require sequential execution
-
-		containerName := "test-container-write-both"
-		testPID := 33333
-
-		// Clean up any existing files
-		t.Cleanup(func() {
-			// Clean up new location
-			if newPath, err := getPIDFilePath(containerName); err == nil {
-				require.NoError(t, os.Remove(newPath))
-			}
-			// Clean up old location
-			oldPath := getOldPIDFilePath(containerName)
-			require.NoError(t, os.Remove(oldPath))
-		})
-
-		// Write PID file
-		require.NoError(t, WritePIDFile(containerName, testPID), "Failed to write PID file")
-
-		// Verify it was written to new location
-		newPath, err := getPIDFilePath(containerName)
-		require.NoError(t, err, "Failed to get new PID file path")
-
-		_, err = os.Stat(newPath)
-		assert.NoError(t, err, "PID file should exist in new location")
-
-		// Verify it was also written to old location for compatibility
-		oldPath := getOldPIDFilePath(containerName)
-		_, err = os.Stat(oldPath)
-		assert.NoError(t, err, "PID file should also exist in old location for version compatibility")
-
-		// Verify both files contain the same PID
-		newPidBytes, err := os.ReadFile(newPath)
-		require.NoError(t, err, "Should read new location file")
-		oldPidBytes, err := os.ReadFile(oldPath)
-		require.NoError(t, err, "Should read old location file")
-		assert.Equal(t, newPidBytes, oldPidBytes, "Both files should contain the same PID")
-	})
-
 	//nolint:paralleltest // File system operations require sequential execution
 	t.Run("RemovePIDFile_RemovesBothLocations", func(t *testing.T) {
 		//nolint:paralleltest // File system operations require sequential execution
@@ -199,37 +159,6 @@ func TestPIDFileBackwardCompatibility(t *testing.T) {
 		assert.True(t, os.IsNotExist(err), "Old PID file should be removed")
 	})
 
-	//nolint:paralleltest // File system operations require sequential execution
-	t.Run("RemovePIDFile_NewFileOnly", func(t *testing.T) {
-		//nolint:paralleltest // File system operations require sequential execution
-
-		containerName := "test-container-new-only"
-		testPID := 66666
-
-		// Clean up any existing files
-		t.Cleanup(func() {
-			// Clean up new location
-			if newPath, err := getPIDFilePath(containerName); err == nil {
-				// Error expected here - ignore.
-				_ = os.Remove(newPath)
-			}
-			// Clean up old location
-			oldPath := getOldPIDFilePath(containerName)
-			// Error expected here - ignore.
-			_ = os.Remove(oldPath)
-		})
-
-		// Test removing when only new file exists
-		require.NoError(t, WritePIDFile(containerName, testPID), "Failed to write PID file")
-
-		err := RemovePIDFile(containerName)
-		assert.NoError(t, err, "Should handle removing only new file")
-
-		newPath, _ := getPIDFilePath(containerName)
-		_, err = os.Stat(newPath)
-		assert.True(t, os.IsNotExist(err), "New PID file should be removed")
-	})
-
 	t.Run("getPIDFilePathWithFallback", func(t *testing.T) {
 		//nolint:paralleltest // File system operations require sequential execution
 
@@ -278,45 +207,6 @@ func TestPIDFileBackwardCompatibility(t *testing.T) {
 //nolint:paralleltest // File system operations require sequential execution
 func TestPIDFileOperations(t *testing.T) {
 
-	t.Run("WriteAndReadPIDFile", func(t *testing.T) {
-		//nolint:paralleltest // File system operations require sequential execution
-
-		containerName := "test-basic-write-read"
-		testPID := 54321
-
-		// Clean up before and after
-		t.Cleanup(func() {
-			require.NoError(t, RemovePIDFile(containerName))
-		})
-
-		// Write PID
-		require.NoError(t, WritePIDFile(containerName, testPID), "Failed to write PID file")
-
-		// Read PID
-		pid, err := ReadPIDFile(containerName)
-		require.NoError(t, err, "Failed to read PID file")
-		assert.Equal(t, testPID, pid, "PID mismatch")
-	})
-
-	t.Run("WriteCurrentPIDFile", func(t *testing.T) {
-		//nolint:paralleltest // File system operations require sequential execution
-
-		containerName := "test-current-pid"
-
-		// Clean up before and after
-		t.Cleanup(func() {
-			require.NoError(t, RemovePIDFile(containerName))
-		})
-
-		// Write current process PID
-		require.NoError(t, WriteCurrentPIDFile(containerName), "Failed to write current PID file")
-
-		// Read and verify
-		pid, err := ReadPIDFile(containerName)
-		require.NoError(t, err, "Failed to read PID file")
-		assert.Equal(t, os.Getpid(), pid, "Should match current process PID")
-	})
-
 	t.Run("ReadNonExistentPIDFile", func(t *testing.T) {
 		//nolint:paralleltest // File system operations require sequential execution
 
@@ -390,49 +280,3 @@ func TestGetPIDFilePath(t *testing.T) {
 			"Old PID file should have correct filename format")
 	})
 }
-
-//nolint:paralleltest // File system operations require sequential execution
-func TestPIDFileMigration(t *testing.T) {
-
-	//nolint:paralleltest // File system operations require sequential execution
-	t.Run("MigrationScenario", func(t *testing.T) {
-		//nolint:paralleltest // File system operations require sequential execution
-
-		containerName := "test-migration"
-		oldPID := 99999
-
-		// Clean up
-		t.Cleanup(func() {
-			require.NoError(t, RemovePIDFile(containerName))
-		})
-
-		// Simulate existing deployment with PID file in old location
-		oldPath := getOldPIDFilePath(containerName)
-		require.NoError(t, os.WriteFile(oldPath, []byte(fmt.Sprintf("%d", oldPID)), 0600),
-			"Failed to create old PID file")
-
-		// New code should still be able to read the old PID
-		pid, err := ReadPIDFile(containerName)
-		require.NoError(t, err, "Should read PID from old location")
-		assert.Equal(t, oldPID, pid, "Should read correct PID from old location")
-
-		// When writing a new PID, it should go to the new location
-		newPID := 88888
-		require.NoError(t, WritePIDFile(containerName, newPID), "Failed to write new PID")
-
-		// Now reading should get the new PID from the new location
-		pid, err = ReadPIDFile(containerName)
-		require.NoError(t, err, "Should read PID from new location")
-		assert.Equal(t, newPID, pid, "Should read new PID from new location")
-
-		// Cleanup should remove both files
-		require.NoError(t, RemovePIDFile(containerName), "Failed to remove PID files")
-
-		_, err = os.Stat(oldPath)
-		assert.True(t, os.IsNotExist(err), "Old file should be removed")
-
-		newPath, _ := getPIDFilePath(containerName)
-		_, err = os.Stat(newPath)
-		assert.True(t, os.IsNotExist(err), "New file should be removed")
-	})
-}

pkg/runner/runner.go

@@ -342,10 +342,6 @@ func (r *Runner) Run(ctx context.Context) error {
 		logger.Infof("MCP server %s stopped", r.Config.ContainerName)
 	}
 
-	// TODO: Stop writing to PID file once we migrate over to statuses.
-	if err := process.WriteCurrentPIDFile(r.Config.BaseName); err != nil {
-		logger.Warnf("Warning: Failed to write PID file: %v", err)
-	}
 	if err := r.statusManager.SetWorkloadPID(ctx, r.Config.BaseName, os.Getpid()); err != nil {
 		logger.Warnf("Warning: Failed to set workload PID: %v", err)
 	}

pkg/workloads/manager.go

@@ -587,10 +587,6 @@ func (d *DefaultManager) RunWorkloadDetached(ctx context.Context, runConfig *run
 	}
 
 	// Write the PID to a file so the stop command can kill the process
-	// TODO: Stop writing to PID file once we migrate over to statuses fully.
-	if err := process.WritePIDFile(runConfig.BaseName, detachedCmd.Process.Pid); err != nil {
-		logger.Warnf("Warning: Failed to write PID file: %v", err)
-	}
 	if err := d.statuses.SetWorkloadPID(ctx, runConfig.BaseName, detachedCmd.Process.Pid); err != nil {
 		logger.Warnf("Failed to set workload %s PID: %v", runConfig.BaseName, err)
 	}

pkg/workloads/statuses/file_status_test.go

@@ -12,7 +12,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/adrg/xdg"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
@@ -1693,25 +1692,14 @@ func TestFileStatusManager_GetWorkload_PIDMigration(t *testing.T) {
 
 	tests := []struct {
 		name            string
-		setupPIDFile    bool
 		pidValue        int
 		workloadStatus  rt.WorkloadStatus
 		processID       int
 		expectMigration bool
 		expectPIDFile   bool // whether PID file should exist after operation
 	}{
-		{
-			name:            "migrates PID when status is running and ProcessID is 0",
-			setupPIDFile:    true,
-			pidValue:        12345,
-			workloadStatus:  rt.WorkloadStatusRunning,
-			processID:       0,
-			expectMigration: true,
-			expectPIDFile:   true, // PID file is NOT deleted (see TODO comment in migration code)
-		},
 		{
 			name:            "no migration when status is not running",
-			setupPIDFile:    true,
 			pidValue:        12345,
 			workloadStatus:  rt.WorkloadStatusStopped,
 			processID:       0,
@@ -1720,7 +1708,6 @@ func TestFileStatusManager_GetWorkload_PIDMigration(t *testing.T) {
 		},
 		{
 			name:            "no migration when ProcessID is not 0",
-			setupPIDFile:    true,
 			pidValue:        12345,
 			workloadStatus:  rt.WorkloadStatusRunning,
 			processID:       98765,
@@ -1729,7 +1716,6 @@ func TestFileStatusManager_GetWorkload_PIDMigration(t *testing.T) {
 		},
 		{
 			name:            "no migration when no PID file exists",
-			setupPIDFile:    false,
 			pidValue:        0,
 			workloadStatus:  rt.WorkloadStatusRunning,
 			processID:       0,
@@ -1770,21 +1756,6 @@ func TestFileStatusManager_GetWorkload_PIDMigration(t *testing.T) {
 			err := manager.setWorkloadStatusInternal(ctx, workloadName, tt.workloadStatus, "test context", &tt.processID)
 			require.NoError(t, err)
 
-			// Setup PID file if needed
-			var pidFilePath string
-			if tt.setupPIDFile {
-				// Create PID file using the process package
-				err = process.WritePIDFile(workloadName, tt.pidValue)
-				require.NoError(t, err)
-
-				// Get the path for cleanup verification
-				// Copy-paste from process package to allow original function to be private.
-				// Since we do not have backwards compatibility requirements for PID file location,
-				// we can simplify the original function to a one-liner.
-				pidFilePath, err = xdg.DataFile(filepath.Join("toolhive", "pids", fmt.Sprintf("toolhive-%s.pid", workloadName)))
-				require.NoError(t, err)
-			}
-
 			// Call GetWorkload which should trigger migration if conditions are met
 			workload, err := manager.GetWorkload(ctx, workloadName)
 			require.NoError(t, err)
@@ -1816,21 +1787,6 @@ func TestFileStatusManager_GetWorkload_PIDMigration(t *testing.T) {
 
 				assert.Equal(t, tt.processID, statusFile.ProcessID, "PID should remain unchanged")
 			}
-
-			// Verify PID file existence
-			if tt.setupPIDFile {
-				_, err := os.Stat(pidFilePath)
-				if tt.expectPIDFile {
-					assert.NoError(t, err, "PID file should still exist")
-				} else {
-					assert.True(t, os.IsNotExist(err), "PID file should be deleted")
-				}
-			}
-
-			// Cleanup
-			if tt.setupPIDFile {
-				_ = process.RemovePIDFile(workloadName)
-			}
 		})
 	}
 }
@@ -1867,14 +1823,6 @@ func TestFileStatusManager_ListWorkloads_PIDMigration(t *testing.T) {
 	err = manager.setWorkloadStatusInternal(ctx, workloadNoMigrate, rt.WorkloadStatusRunning, "running", &existingPID)
 	require.NoError(t, err)
 
-	// Create PID files for both workloads
-	migrationPID := 12345
-	err = process.WritePIDFile(workloadMigrate, migrationPID)
-	require.NoError(t, err)
-
-	err = process.WritePIDFile(workloadNoMigrate, 99999)
-	require.NoError(t, err)
-
 	// Call ListWorkloads
 	workloads, err := manager.ListWorkloads(ctx, true, nil)
 	require.NoError(t, err)
@@ -1908,7 +1856,6 @@ func TestFileStatusManager_ListWorkloads_PIDMigration(t *testing.T) {
 	var statusFile1 workloadStatusFile
 	err = json.Unmarshal(data1, &statusFile1)
 	require.NoError(t, err)
-	assert.Equal(t, migrationPID, statusFile1.ProcessID, "PID should be migrated for first workload")
 
 	// Verify no migration for second workload
 	statusFilePath2 := manager.getStatusFilePath(workloadNoMigrate)