From 5b99e56941284665ba0fb092618395f4718aab75 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Sat, 25 Oct 2025 12:35:32 -0700 Subject: [PATCH] =?UTF-8?q?Refactor=20tryAutoStartDaemon=20to=20reduce=20c?= =?UTF-8?q?omplexity=20(34=20=E2=86=92=20<10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .beads/beads.jsonl | 2 +- cmd/bd/main.go | 203 +++++++++++++++++++++++---------------------- 2 files changed, 105 insertions(+), 100 deletions(-) diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index 12dce632..79744632 100644 --- 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"}]} diff --git a/cmd/bd/main.go b/cmd/bd/main.go index 3283548a..1daeb5b0 100644 --- 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