Merge pull request #1017 from kevinWangSheng/fix/instance-lock-kill-existing-waits-for-exit

fix(instance_lock): wait for killed process to exit so --force can re-acquire the flock

cmd/cc-connect/instance_lock.go

@@ -7,8 +7,22 @@ import (
 	"os"
 	"path/filepath"
 	"syscall"
+	"time"
 )
 
+// killWaitTimeout caps how long KillExistingInstance polls for the
+// SIGKILLed process to actually be reaped by the kernel. SIGKILL is
+// async, and the file lock isn't released until the process truly
+// exits — so without this wait, `cc-connect --force` would race the
+// kernel and trip "another instance running" against the lock that
+// the dying process still holds.
+const killWaitTimeout = 5 * time.Second
+
+// killWaitInterval is the poll interval between Signal(0) probes
+// while waiting for the killed process to exit. Short enough to
+// minimise the user-visible delay on a clean kill.
+const killWaitInterval = 25 * time.Millisecond
+
 // InstanceLock provides a file-based exclusive lock to prevent multiple
 // cc-connect instances with the same config from running simultaneously.
 type InstanceLock struct {
@@ -139,7 +153,26 @@ func KillExistingInstance(configPath string) bool {
 		return false
 	}
 
-	// Wait a moment for the process to exit
-	// Note: we can't use proc.Wait() as we're not the parent
+	// Wait for the process to actually exit before returning. SIGKILL
+	// is async — proc.Kill() just queues the signal, and the file lock
+	// stays held until the kernel reaps the process. Without this poll
+	// the immediately-following AcquireInstanceLock would race against
+	// the still-held flock and surface a confusing "already running"
+	// error after --force.
+	//
+	// We can't use proc.Wait() because we're not the parent; use
+	// Signal(0) instead, which fails with ESRCH once the process is
+	// gone.
+	deadline := time.Now().Add(killWaitTimeout)
+	for time.Now().Before(deadline) {
+		if err := proc.Signal(syscall.Signal(0)); err != nil {
+			return true
+		}
+		time.Sleep(killWaitInterval)
+	}
+	// Returning true anyway: the process didn't reap inside the
+	// timeout (very unusual for SIGKILL), but the caller's downstream
+	// AcquireInstanceLock will surface a clear error rather than
+	// silent corruption.
 	return true
 }

cmd/cc-connect/instance_lock_test.go

@@ -3,8 +3,12 @@
 package main
 
 import (
+	"fmt"
+	"os"
+	"os/exec"
 	"path/filepath"
 	"testing"
+	"time"
 )
 
 func TestAcquireInstanceLock_Success(t *testing.T) {
@@ -40,3 +44,84 @@ func TestAcquireInstanceLock_AlreadyLocked(t *testing.T) {
 		t.Fatal("second AcquireInstanceLock should fail while lock held")
 	}
 }
+
+// TestMain doubles as the helper-process entry point for
+// TestKillExistingInstance_WaitsForExit. When INSTANCE_LOCK_HELPER
+// is set, the binary acquires the lock at the given path and blocks
+// until SIGKILL; otherwise it runs the normal test suite.
+func TestMain(m *testing.M) {
+	if cfg := os.Getenv("INSTANCE_LOCK_HELPER"); cfg != "" {
+		lock, err := AcquireInstanceLock(cfg)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "helper: AcquireInstanceLock: %v\n", err)
+			os.Exit(2)
+		}
+		// Signal readiness to the parent by writing the PID file
+		// (already done inside AcquireInstanceLock) and parking until
+		// the parent SIGKILLs us. select{} blocks forever; SIGKILL
+		// terminates the process immediately and releases the flock.
+		_ = lock
+		select {}
+	}
+	os.Exit(m.Run())
+}
+
+// TestKillExistingInstance_WaitsForExit pins the bug where
+// KillExistingInstance returned immediately after proc.Kill() — only
+// queuing SIGKILL, not waiting for the kernel to reap the child and
+// release the file lock. With the fix the function polls Signal(0)
+// until the process is gone, so an immediate-following
+// AcquireInstanceLock can succeed against the freed flock (which is
+// exactly the `cc-connect --force` user flow).
+func TestKillExistingInstance_WaitsForExit(t *testing.T) {
+	dir := t.TempDir()
+	cfg := filepath.Join(dir, "config.toml")
+
+	exe, err := os.Executable()
+	if err != nil {
+		t.Fatalf("os.Executable: %v", err)
+	}
+
+	// Spawn the helper, which acquires the lock and blocks.
+	helper := exec.Command(exe, "-test.run=TestMain$")
+	helper.Env = append(os.Environ(), "INSTANCE_LOCK_HELPER="+cfg)
+	if err := helper.Start(); err != nil {
+		t.Fatalf("start helper: %v", err)
+	}
+	// Best-effort cleanup if KillExistingInstance returns false.
+	t.Cleanup(func() {
+		if helper.Process != nil {
+			_ = helper.Process.Kill()
+			_, _ = helper.Process.Wait()
+		}
+	})
+
+	// Wait for the helper to acquire the lock. Poll the lock file
+	// until it contains a non-zero PID. Bound to avoid hanging if the
+	// helper crashes.
+	lockPath := filepath.Join(dir, ".config.toml.lock")
+	deadline := time.Now().Add(3 * time.Second)
+	for time.Now().Before(deadline) {
+		if pid := readPIDFromLockFile(lockPath); pid > 0 {
+			break
+		}
+		time.Sleep(25 * time.Millisecond)
+	}
+	if pid := readPIDFromLockFile(lockPath); pid <= 0 {
+		t.Fatal("helper did not acquire lock in time")
+	}
+
+	if !KillExistingInstance(cfg) {
+		t.Fatal("KillExistingInstance returned false")
+	}
+
+	// Immediately after KillExistingInstance returns, the lock MUST
+	// be acquirable. Without the wait-for-exit fix, this would race
+	// the kernel and frequently fail because the SIGKILLed helper's
+	// flock is still held.
+	lock, err := AcquireInstanceLock(cfg)
+	if err != nil {
+		t.Fatalf("AcquireInstanceLock immediately after KillExistingInstance: %v", err)
+	}
+	lock.Release()
+}