Refactor tryAutoStartDaemon to reduce complexity (34 → <10)

Extracted 7 helper functions: - debugLog: Centralized debug logging - isDaemonHealthy: Fast-path health check - acquireStartLock: Lock acquisition with wait/retry - handleStaleLock: Stale lock detection - handleExistingSocket: Socket cleanup/validation - determineSocketMode: Global vs local daemon logic - startDaemonProcess: Process spawning and readiness - setupDaemonIO: I/O redirection Ref: bd-55
2025-10-25 12:35:32 -07:00
parent 36c1c648fc
commit 5b99e56941
2 changed files with 105 additions and 100 deletions
--- a/.beads/beads.jsonl
+++ b/.beads/beads.jsonl
@@ -82,7 +82,7 @@
 {"id":"bd-52","title":"Clean up linter errors (914 total issues)","description":"The codebase has 914 linter issues reported by golangci-lint. While many are documented as baseline in LINTING.md, we should clean these up systematically to improve code quality and maintainability.","design":"Break down by linter category, prioritizing high-impact issues:\n1. dupl (7) - Code duplication\n2. goconst (12) - Repeated strings\n3. gocyclo (11) - High complexity functions\n4. revive (78) - Style issues\n5. gosec (102) - Security warnings\n6. errcheck (683) - Unchecked errors (many in tests)","acceptance_criteria":"All linter categories reduced to acceptable levels, with remaining baseline documented in LINTING.md","notes":"Reduced from 56 to 41 issues locally, then to 0 issues.\n\n**Fixed in commits:**\n- c2c7eda: Fixed 15 actual errors (dupl, gosec, revive, staticcheck, unparam)\n- 963181d: Configured exclusions to get to 0 issues locally\n\n**Current status:**\n- ✅ Local: golangci-lint reports 0 issues\n- ❌ CI: Still failing (see bd-65)\n\n**Problem:**\nConfig v2 format or golangci-lint-action@v8 compatibility issue causing CI to fail despite local success.\n\n**Next:** Debug bd-65 to fix CI/local discrepancy","status":"in_progress","priority":2,"issue_type":"epic","created_at":"2025-10-24T01:01:12.997982-07:00","updated_at":"2025-10-24T13:51:54.439577-07:00"}
 {"id":"bd-53","title":"Fix code duplication in label.go (dupl)","description":"Lines 72-120 duplicate lines 122-170 in cmd/bd/label.go. The add and remove commands have nearly identical structure.","design":"Extract common batch operation logic into a shared helper function that takes the operation type as a parameter.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-24T01:01:36.971666-07:00","updated_at":"2025-10-24T13:51:54.416434-07:00","closed_at":"2025-10-24T12:40:43.046348-07:00","dependencies":[{"issue_id":"bd-53","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.325899-07:00","created_by":"renumber"}]}
 {"id":"bd-54","title":"Convert repeated strings to constants (goconst)","description":"12 instances of repeated strings that should be constants: \"alice\", \"windows\", \"bd-114\", \"daemon\", \"import\", \"healthy\", \"unhealthy\", \"1.0.0\", \"custom-1\", \"custom-2\"","design":"Create package-level or test-level constants for frequently used test strings and command names.","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-24T01:01:36.9778-07:00","updated_at":"2025-10-24T13:51:54.439751-07:00","dependencies":[{"issue_id":"bd-54","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.326123-07:00","created_by":"renumber"}]}
-{"id":"bd-55","title":"Refactor high complexity functions (gocyclo)","description":"11 functions exceed cyclomatic complexity threshold (\u003e30): runDaemonLoop (42), importIssuesCore (71), TestLabelCommands (67), issueDataChanged (39), etc.","design":"Break down complex functions into smaller, testable units. Extract validation, error handling, and business logic into separate functions.","notes":"Refactored issueDataChanged from complexity 39 → 11 by extracting into fieldComparator struct with methods for each comparison type.\n\nRefactored runDaemonLoop from complexity 42 → 7 by extracting:\n- setupDaemonLogger: Logger initialization logic\n- setupDaemonLock: Lock and PID file management\n- startRPCServer: RPC server startup with error handling\n- runGlobalDaemon: Global daemon mode handling\n- createSyncFunc: Sync cycle logic (export, commit, pull, import, push)\n- runEventLoop: Signal handling and main event loop\n\nCode review fixes:\n- Fixed sync overlap: Changed initial sync from `go doSync()` to synchronous `doSync()` to prevent race with ticker\n- Fixed resource cleanup: Replaced `os.Exit(1)` with `return` after acquiring locks to ensure defers run and clean up PID files/locks\n- Added signal.Stop(sigChan) in runEventLoop and runGlobalDaemon to prevent lingering notifications\n- Added server.Stop() in serverErrChan case for consistent cleanup\n\nRefactored TestLabelCommands from complexity 67 → \u003c10 by extracting labelTestHelper with methods:\n- createIssue: Issue creation helper\n- addLabel/addLabels: Label addition helpers\n- removeLabel: Label removal helper\n- getLabels: Label retrieval helper\n- assertLabelCount/assertHasLabel/assertHasLabels/assertNotHasLabel: Assertion helpers\n- assertLabelEvent: Event verification helper\n\nRefactored TestReopenCommand from complexity 37 → \u003c10 by extracting reopenTestHelper with methods:\n- createIssue: Issue creation helper\n- closeIssue/reopenIssue: State transition helpers\n- getIssue: Issue retrieval helper\n- addComment: Comment addition helper\n- assertStatus/assertClosedAtSet/assertClosedAtNil: Status assertion helpers\n- assertCommentEvent: Event verification helper\n\nAll tests pass after refactoring.","status":"in_progress","priority":1,"issue_type":"task","created_at":"2025-10-24T01:01:36.989066-07:00","updated_at":"2025-10-25T11:22:17.056061-07:00","dependencies":[{"issue_id":"bd-55","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.323992-07:00","created_by":"renumber"}]}
+{"id":"bd-55","title":"Refactor high complexity functions (gocyclo)","description":"11 functions exceed cyclomatic complexity threshold (\u003e30): runDaemonLoop (42), importIssuesCore (71), TestLabelCommands (67), issueDataChanged (39), etc.","design":"Break down complex functions into smaller, testable units. Extract validation, error handling, and business logic into separate functions.","notes":"Refactored issueDataChanged from complexity 39 → 11 by extracting into fieldComparator struct with methods for each comparison type.\n\nRefactored runDaemonLoop from complexity 42 → 7 by extracting:\n- setupDaemonLogger: Logger initialization logic\n- setupDaemonLock: Lock and PID file management\n- startRPCServer: RPC server startup with error handling\n- runGlobalDaemon: Global daemon mode handling\n- createSyncFunc: Sync cycle logic (export, commit, pull, import, push)\n- runEventLoop: Signal handling and main event loop\n\nCode review fixes:\n- Fixed sync overlap: Changed initial sync from `go doSync()` to synchronous `doSync()` to prevent race with ticker\n- Fixed resource cleanup: Replaced `os.Exit(1)` with `return` after acquiring locks to ensure defers run and clean up PID files/locks\n- Added signal.Stop(sigChan) in runEventLoop and runGlobalDaemon to prevent lingering notifications\n- Added server.Stop() in serverErrChan case for consistent cleanup\n\nRefactored TestLabelCommands from complexity 67 → \u003c10 by extracting labelTestHelper with methods:\n- createIssue: Issue creation helper\n- addLabel/addLabels: Label addition helpers\n- removeLabel: Label removal helper\n- getLabels: Label retrieval helper\n- assertLabelCount/assertHasLabel/assertHasLabels/assertNotHasLabel: Assertion helpers\n- assertLabelEvent: Event verification helper\n\nRefactored TestReopenCommand from complexity 37 → \u003c10 by extracting reopenTestHelper with methods:\n- createIssue: Issue creation helper\n- closeIssue/reopenIssue: State transition helpers\n- getIssue: Issue retrieval helper\n- addComment: Comment addition helper\n- assertStatus/assertClosedAtSet/assertClosedAtNil: Status assertion helpers\n- assertCommentEvent: Event verification helper\n\nAll tests pass after refactoring.\n\n**Remaining functions (7):**\n1. TestLibraryIntegration (beads_integration_test.go:14) - complexity 32\n2. TestExportImport (cmd/bd/export_import_test.go:17) - complexity 31\n3. TestListCommand (cmd/bd/list_test.go:15) - complexity 31\n4. tryAutoStartDaemon (cmd/bd/main.go:625) - complexity 34\n5. TestGetEpicsEligibleForClosure (internal/storage/sqlite/epics_test.go:10) - complexity 32\n6. DeleteIssues (internal/storage/sqlite/sqlite.go:1466) - complexity 37\n7. TestCreateIssues (internal/storage/sqlite/sqlite_test.go:195) - complexity 35","status":"in_progress","priority":1,"issue_type":"task","created_at":"2025-10-24T01:01:36.989066-07:00","updated_at":"2025-10-25T12:19:23.537919-07:00","dependencies":[{"issue_id":"bd-55","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.323992-07:00","created_by":"renumber"}]}
 {"id":"bd-56","title":"Fix revive style issues (78 issues)","description":"Style violations: unused parameters (many cmd/args in cobra commands), missing exported comments, stuttering names (SQLiteStorage), indent-error-flow issues.","design":"Rename unused params to _, add godoc comments to exported types, fix stuttering names, simplify control flow.","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-24T01:01:36.99984-07:00","updated_at":"2025-10-24T13:51:54.417341-07:00","dependencies":[{"issue_id":"bd-56","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.322412-07:00","created_by":"renumber"}]}
 {"id":"bd-57","title":"Address gosec security warnings (102 issues)","description":"Security linter warnings: file permissions (0755 should be 0750), G304 file inclusion via variable, G204 subprocess launches. Many are false positives but should be reviewed.","design":"Review each gosec warning. Add exclusions for legitimate cases to .golangci.yml. Fix real security issues (overly permissive file modes).","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-24T01:01:37.0139-07:00","updated_at":"2025-10-24T13:51:54.417632-07:00","dependencies":[{"issue_id":"bd-57","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.324202-07:00","created_by":"renumber"}]}
 {"id":"bd-58","title":"Handle unchecked errors (errcheck - 683 issues)","description":"683 unchecked error returns, mostly in tests (Close, Rollback, RemoveAll). Many already excluded in config but still showing up.","design":"Review .golangci.yml exclude-rules. Most defer Close/Rollback errors in tests can be ignored. Add systematic exclusions or explicit _ = assignments where appropriate.","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-24T01:01:37.018404-07:00","updated_at":"2025-10-24T13:51:54.41793-07:00","dependencies":[{"issue_id":"bd-58","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.324423-07:00","created_by":"renumber"}]}
--- a/cmd/bd/main.go
+++ b/cmd/bd/main.go
@@ -623,110 +623,119 @@ func isDaemonRunningQuiet(pidFile string) bool {
 // tryAutoStartDaemon attempts to start the daemon in the background
 // Returns true if daemon was started successfully and socket is ready
 func tryAutoStartDaemon(socketPath string) bool {
-	// Check if we've failed recently (exponential backoff)
 	if !canRetryDaemonStart() {
-		if os.Getenv("BD_DEBUG") != "" {
-			fmt.Fprintf(os.Stderr, "Debug: skipping auto-start due to recent failures\n")
-		}
+		debugLog("skipping auto-start due to recent failures")
 		return false
 	}

-	// Fast path: check if daemon is already healthy
-	client, err := rpc.TryConnect(socketPath)
-	if err == nil && client != nil {
-		_ = client.Close()
-		if os.Getenv("BD_DEBUG") != "" {
-			fmt.Fprintf(os.Stderr, "Debug: daemon already running and healthy\n")
-		}
+	if isDaemonHealthy(socketPath) {
+		debugLog("daemon already running and healthy")
 		return true
 	}

-	// Use lockfile to prevent multiple processes from starting daemon simultaneously
 	lockPath := socketPath + ".startlock"
+	if !acquireStartLock(lockPath, socketPath) {
+		return false
+	}
+	defer os.Remove(lockPath)
+
+	if handleExistingSocket(socketPath) {
+		return true
+	}
+
+	socketPath, isGlobal := determineSocketMode(socketPath)
+	return startDaemonProcess(socketPath, isGlobal)
+}
+
+func debugLog(msg string, args ...interface{}) {
+	if os.Getenv("BD_DEBUG") != "" {
+		fmt.Fprintf(os.Stderr, "Debug: "+msg+"\n", args...)
+	}
+}
+
+func isDaemonHealthy(socketPath string) bool {
+	client, err := rpc.TryConnect(socketPath)
+	if err == nil && client != nil {
+		_ = client.Close()
+		return true
+	}
+	return false
+}
+
+func acquireStartLock(lockPath, socketPath string) bool {
 	lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600)
 	if err != nil {
-		// Someone else is starting daemon, wait for socket readiness
-		if os.Getenv("BD_DEBUG") != "" {
-			fmt.Fprintf(os.Stderr, "Debug: another process is starting daemon, waiting for readiness\n")
-		}
+		debugLog("another process is starting daemon, waiting for readiness")
 		if waitForSocketReadiness(socketPath, 5*time.Second) {
 			return true
 		}
+		return handleStaleLock(lockPath, socketPath)
+	}

-		// Socket still not ready - check if lock is stale
-		if lockPID, err := readPIDFromFile(lockPath); err == nil {
-			if !isPIDAlive(lockPID) {
-				if os.Getenv("BD_DEBUG") != "" {
-					fmt.Fprintf(os.Stderr, "Debug: lock is stale (PID %d dead), removing and retrying\n", lockPID)
-				}
-				_ = os.Remove(lockPath)
-				// Retry once
-				return tryAutoStartDaemon(socketPath)
-			}
-		}
+	_, _ = fmt.Fprintf(lockFile, "%d\n", os.Getpid())
+	_ = lockFile.Close()
+	return true
+}
+
+func handleStaleLock(lockPath, socketPath string) bool {
+	lockPID, err := readPIDFromFile(lockPath)
+	if err == nil && !isPIDAlive(lockPID) {
+		debugLog("lock is stale (PID %d dead), removing and retrying", lockPID)
+		_ = os.Remove(lockPath)
+		return tryAutoStartDaemon(socketPath)
+	}
+	return false
+}
+
+func handleExistingSocket(socketPath string) bool {
+	if _, err := os.Stat(socketPath); err != nil {
 		return false
 	}

-	// Write our PID to lockfile
-	_, _ = fmt.Fprintf(lockFile, "%d\n", os.Getpid())
-	_ = lockFile.Close()
-	defer func() { _ = os.Remove(lockPath) }()
+	if canDialSocket(socketPath, 200*time.Millisecond) {
+		debugLog("daemon started by another process")
+		return true
+	}

-	// Under lock: check for stale socket and clean up if necessary
-	if _, err := os.Stat(socketPath); err == nil {
-		// Socket exists - check if it's truly stale by trying a quick connect
-		if canDialSocket(socketPath, 200*time.Millisecond) {
-			// Another daemon is running - it must have started between our check and lock acquisition
-			if os.Getenv("BD_DEBUG") != "" {
-				fmt.Fprintf(os.Stderr, "Debug: daemon started by another process\n")
-			}
-			return true
-		}
-
-		// Socket exists but not responding - check if PID is alive before removing
-		pidFile := getPIDFileForSocket(socketPath)
-		if pidFile != "" {
-			if pid, err := readPIDFromFile(pidFile); err == nil && isPIDAlive(pid) {
-				// Daemon process is alive but socket not responding - wait for it
-				if os.Getenv("BD_DEBUG") != "" {
-					fmt.Fprintf(os.Stderr, "Debug: daemon PID %d alive, waiting for socket\n", pid)
-				}
-				return waitForSocketReadiness(socketPath, 5*time.Second)
-			}
-		}
-
-		// Socket is stale (connect failed and PID dead/missing) - safe to remove
-		if os.Getenv("BD_DEBUG") != "" {
-			fmt.Fprintf(os.Stderr, "Debug: socket is stale, cleaning up\n")
-		}
-		_ = os.Remove(socketPath)
-		if pidFile != "" {
-			_ = os.Remove(pidFile)
+	pidFile := getPIDFileForSocket(socketPath)
+	if pidFile != "" {
+		if pid, err := readPIDFromFile(pidFile); err == nil && isPIDAlive(pid) {
+			debugLog("daemon PID %d alive, waiting for socket", pid)
+			return waitForSocketReadiness(socketPath, 5*time.Second)
 		}
 	}

-	// Determine if we should start global or local daemon
-	// If requesting local socket, check if we should suggest global instead
-	isGlobal := false
-	if home, err := os.UserHomeDir(); err == nil {
-		globalSocket := filepath.Join(home, ".beads", "bd.sock")
-		if socketPath == globalSocket {
-			isGlobal = true
-		} else if shouldUseGlobalDaemon() {
-			// User has multiple repos, but requested local daemon
-			// Auto-start global daemon instead and log suggestion
-			isGlobal = true
-			socketPath = globalSocket
-			if os.Getenv("BD_DEBUG") != "" {
-				fmt.Fprintf(os.Stderr, "Debug: detected multiple repos, auto-starting global daemon\n")
-			}
-		}
+	debugLog("socket is stale, cleaning up")
+	_ = os.Remove(socketPath)
+	if pidFile != "" {
+		_ = os.Remove(pidFile)
+	}
+	return false
+}
+
+func determineSocketMode(socketPath string) (string, bool) {
+	home, err := os.UserHomeDir()
+	if err != nil {
+		return socketPath, false
 	}

-	// Build daemon command using absolute path for security
+	globalSocket := filepath.Join(home, ".beads", "bd.sock")
+	if socketPath == globalSocket {
+		return socketPath, true
+	}
+
+	if shouldUseGlobalDaemon() {
+		debugLog("detected multiple repos, auto-starting global daemon")
+		return globalSocket, true
+	}
+
+	return socketPath, false
+}
+
+func startDaemonProcess(socketPath string, isGlobal bool) bool {
 	binPath, err := os.Executable()
 	if err != nil {
-		binPath = os.Args[0] // Fallback
+		binPath = os.Args[0]
 	}

 	args := []string{"daemon"}
@@ -734,49 +743,45 @@ func tryAutoStartDaemon(socketPath string) bool {
 		args = append(args, "--global")
 	}

-	// Start daemon in background with proper I/O redirection
 	cmd := exec.Command(binPath, args...)
+	setupDaemonIO(cmd)

-	// Redirect stdio to /dev/null to prevent daemon output in foreground
-	devNull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0)
-	if err == nil {
-		cmd.Stdout = devNull
-		cmd.Stderr = devNull
-		cmd.Stdin = devNull
-		defer devNull.Close()
-	}
-
-	// Set working directory to database directory for local daemon
 	if !isGlobal && dbPath != "" {
 		cmd.Dir = filepath.Dir(dbPath)
 	}

-	// Detach from parent process
 	configureDaemonProcess(cmd)
 	if err := cmd.Start(); err != nil {
 		recordDaemonStartFailure()
-		if os.Getenv("BD_DEBUG") != "" {
-			fmt.Fprintf(os.Stderr, "Debug: failed to start daemon: %v\n", err)
-		}
+		debugLog("failed to start daemon: %v", err)
 		return false
 	}

-	// Reap the process to avoid zombies
 	go func() { _ = cmd.Wait() }()

-	// Wait for socket to be ready with actual connection test
 	if waitForSocketReadiness(socketPath, 5*time.Second) {
 		recordDaemonStartSuccess()
 		return true
 	}

 	recordDaemonStartFailure()
-	if os.Getenv("BD_DEBUG") != "" {
-		fmt.Fprintf(os.Stderr, "Debug: daemon socket not ready after 5 seconds\n")
-	}
+	debugLog("daemon socket not ready after 5 seconds")
 	return false
 }

+func setupDaemonIO(cmd *exec.Cmd) {
+	devNull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0)
+	if err == nil {
+		cmd.Stdout = devNull
+		cmd.Stderr = devNull
+		cmd.Stdin = devNull
+		go func() {
+			time.Sleep(1 * time.Second)
+			devNull.Close()
+		}()
+	}
+}
+
 // getPIDFileForSocket returns the PID file path for a given socket path
 func getPIDFileForSocket(socketPath string) string {
 	// PID file is in same directory as socket, named daemon.pid