bugfixes nov2025 pt3 (#6)

* Minor improvements. * DRY the codebase. * Add version checker / updater.
2026-07-07 06:25:43 +00:00 · 2025-11-25 01:28:23 +00:00
parent 49acba5679
commit 3a7cc6f502
14 changed files with 634 additions and 158 deletions
@@ -6,11 +6,20 @@ import (
 	"os/exec"
 	"runtime"
 	"strings"
+
+	"github.com/nvm/kportal/internal/logger"
+)
+
+const (
+	// maxPIDLength is the maximum length of a valid PID string (9 digits covers PIDs up to 999,999,999)
+	maxPIDLength = 9
+	// minNetstatFields is the minimum number of fields expected in netstat output
+	minNetstatFields = 5
 )

 // isValidPID validates that a PID string contains only digits
 func isValidPID(pid string) bool {
-	if len(pid) == 0 || len(pid) > 9 {
+	if len(pid) == 0 || len(pid) > maxPIDLength {
 		return false
 	}
 	for _, c := range pid {
@@ -21,6 +30,72 @@ func isValidPID(pid string) bool {
 	return true
 }

+// processInfo holds information about a process using a port
+type processInfo struct {
+	pid     string
+	name    string
+	isValid bool
+}
+
+// formatProcessInfo formats process information for display
+func formatProcessInfo(info processInfo) string {
+	if !info.isValid {
+		return "unknown"
+	}
+	if info.name != "" {
+		return fmt.Sprintf("%s (PID %s)", info.name, info.pid)
+	}
+	return fmt.Sprintf("PID %s", info.pid)
+}
+
+// formatProcessList formats a list of processes into a human-readable string.
+// Returns "unknown" if the list is empty.
+func formatProcessList(processes []processInfo) string {
+	if len(processes) == 0 {
+		return "unknown"
+	}
+	if len(processes) == 1 {
+		return formatProcessInfo(processes[0])
+	}
+	// Multiple processes - format as comma-separated list
+	parts := make([]string, len(processes))
+	for i, p := range processes {
+		parts[i] = formatProcessInfo(p)
+	}
+	return strings.Join(parts, ", ")
+}
+
+// getProcessNameByPID retrieves the process name for a given PID on Unix systems
+func getProcessNameByPID(pid string) string {
+	cmd := exec.Command("ps", "-p", pid, "-o", "comm=")
+	output, err := cmd.Output()
+	if err != nil {
+		return ""
+	}
+	return strings.TrimSpace(string(output))
+}
+
+// getProcessNameByPIDWindows retrieves the process name for a given PID on Windows
+func getProcessNameByPIDWindows(pid string) string {
+	cmd := exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %s", pid), "/FO", "CSV", "/NH")
+	output, err := cmd.Output()
+	if err != nil {
+		return ""
+	}
+
+	// Parse CSV output: "process.exe","1234","Console","1","12,345 K"
+	csvLine := strings.TrimSpace(string(output))
+	if csvLine == "" {
+		return ""
+	}
+
+	parts := strings.Split(csvLine, ",")
+	if len(parts) > 0 {
+		return strings.Trim(parts[0], "\"")
+	}
+	return ""
+}
+
 // PortConflict represents a local port that is already in use.
 type PortConflict struct {
 	Port     int    // The conflicting port number
@@ -102,27 +177,55 @@ func (pc *PortChecker) getProcessUsingPortUnix(port int) string {
 		return "unknown"
 	}

-	// Get the first PID if multiple are returned
+	// Handle multiple PIDs (multiple processes on same port)
 	pids := strings.Split(pidStr, "\n")
-	pid := pids[0]
+	var validProcesses []processInfo

-	if !isValidPID(pid) {
-		return "unknown"
+	for _, pid := range pids {
+		pid = strings.TrimSpace(pid)
+		if pid == "" {
+			continue
+		}
+
+		if !isValidPID(pid) {
+			logger.Debug("Invalid PID format from lsof output", map[string]interface{}{
+				"port":    port,
+				"raw_pid": pid,
+			})
+			continue
+		}
+
+		procName := getProcessNameByPID(pid)
+		validProcesses = append(validProcesses, processInfo{
+			pid:     pid,
+			name:    procName,
+			isValid: true,
+		})
 	}

-	// Get process name using ps
-	cmd = exec.Command("ps", "-p", pid, "-o", "comm=")
-	output, err = cmd.Output()
-	if err != nil {
-		return fmt.Sprintf("PID %s", pid)
+	return formatProcessList(validProcesses)
+}
+
+// isListeningState checks if a netstat line indicates a listening state.
+// This handles both English and potentially other locales by checking for common patterns.
+func isListeningState(line string, fields []string) bool {
+	upperLine := strings.ToUpper(line)
+
+	// Check for common listening state indicators across locales
+	// English: LISTENING, German: ABHÖREN, French: ÉCOUTE, etc.
+	// The most reliable check is the state field position (4th field, 0-indexed = 3)
+	// and that it's a TCP connection with 0.0.0.0:0 or *:* as foreign address
+	if len(fields) >= minNetstatFields {
+		state := strings.ToUpper(fields[3])
+		// Common listening state values across Windows locales
+		if state == "LISTENING" || state == "ABHÖREN" || state == "ÉCOUTE" ||
+			state == "ESCUCHANDO" || state == "ASCOLTO" || state == "NASŁUCHIWANIE" {
+			return true
+		}
 	}

-	procName := strings.TrimSpace(string(output))
-	if procName == "" {
-		return fmt.Sprintf("PID %s", pid)
-	}
-
-	return fmt.Sprintf("%s (PID %s)", procName, pid)
+	// Fallback: check if line contains LISTENING (most common case)
+	return strings.Contains(upperLine, "LISTENING")
 }

 // getProcessUsingPortWindows uses netstat to find the process using a port on Windows.
@@ -138,6 +241,8 @@ func (pc *PortChecker) getProcessUsingPortWindows(port int) string {
 	lines := strings.Split(string(output), "\n")
 	portStr := fmt.Sprintf(":%d", port)

+	var validProcesses []processInfo
+
 	for _, line := range lines {
 		if !strings.Contains(line, portStr) {
 			continue
@@ -146,44 +251,42 @@ func (pc *PortChecker) getProcessUsingPortWindows(port int) string {
 		// Parse the line to extract PID
 		// Format: TCP    0.0.0.0:8080    0.0.0.0:0    LISTENING    1234
 		fields := strings.Fields(line)
-		if len(fields) < 5 {
+		if len(fields) < minNetstatFields {
 			continue
 		}

-		// Check if this is a LISTENING state
-		if !strings.Contains(strings.ToUpper(line), "LISTENING") {
+		// Check if this is a LISTENING state (locale-aware)
+		if !isListeningState(line, fields) {
+			continue
+		}
+
+		// Verify the local address field actually contains our port
+		// (avoid matching port in foreign address)
+		localAddr := fields[1]
+		if !strings.HasSuffix(localAddr, portStr) {
 			continue
 		}

 		pid := fields[len(fields)-1]

 		if !isValidPID(pid) {
-			return "unknown"
+			logger.Debug("Invalid PID format from netstat output", map[string]interface{}{
+				"port":    port,
+				"raw_pid": pid,
+				"line":    line,
+			})
+			continue
 		}

-		// Get process name using tasklist
-		cmd = exec.Command("tasklist", "/FI", fmt.Sprintf("PID eq %s", pid), "/FO", "CSV", "/NH")
-		output, err = cmd.Output()
-		if err != nil {
-			return fmt.Sprintf("PID %s", pid)
-		}
-
-		// Parse CSV output: "process.exe","1234","Console","1","12,345 K"
-		csvLine := strings.TrimSpace(string(output))
-		if csvLine == "" {
-			return fmt.Sprintf("PID %s", pid)
-		}
-
-		parts := strings.Split(csvLine, ",")
-		if len(parts) > 0 {
-			procName := strings.Trim(parts[0], "\"")
-			return fmt.Sprintf("%s (PID %s)", procName, pid)
-		}
-
-		return fmt.Sprintf("PID %s", pid)
+		procName := getProcessNameByPIDWindows(pid)
+		validProcesses = append(validProcesses, processInfo{
+			pid:     pid,
+			name:    procName,
+			isValid: true,
+		})
 	}

-	return "unknown"
+	return formatProcessList(validProcesses)
 }

 // FormatConflicts formats port conflicts into a human-readable error message.
@@ -123,11 +123,18 @@ func (w *Watchdog) monitorLoop() {
 	}
 }

+// hungWorkerInfo stores information about a hung worker for deferred callback execution
+type hungWorkerInfo struct {
+	forwardID string
+	callback  func(string)
+}
+
 // checkWorkers checks all registered workers for hung state
 func (w *Watchdog) checkWorkers() {
-	w.mu.Lock()
-	defer w.mu.Unlock()
+	// Collect hung workers while holding the lock
+	var hungWorkers []hungWorkerInfo

+	w.mu.Lock()
 	now := time.Now()
 	for forwardID, state := range w.workers {
 		timeSinceHeartbeat := now.Sub(state.lastHeartbeat)
@@ -145,14 +152,23 @@ func (w *Watchdog) checkWorkers() {
 					"heartbeat_count":      state.heartbeatCount,
 				})

-				// Trigger callback to handle hung worker (without holding lock)
+				// Collect callback for deferred execution outside the lock
 				if state.onHungCallback != nil {
-					callback := state.onHungCallback
-					w.mu.Unlock()
-					callback(forwardID)
-					w.mu.Lock()
+					hungWorkers = append(hungWorkers, hungWorkerInfo{
+						forwardID: forwardID,
+						callback:  state.onHungCallback,
+					})
 				}
 			}
 		}
 	}
+	w.mu.Unlock()
+
+	// Execute callbacks outside the lock to prevent deadlocks and ensure
+	// consistent state during callback execution. Callbacks are idempotent
+	// (they trigger reconnection via channels), so concurrent state changes
+	// between detection and callback execution are safe.
+	for _, hw := range hungWorkers {
+		hw.callback(hw.forwardID)
+	}
 }