From 1351dc7562fdf919b0ede3c9b0117aff8bf1506f Mon Sep 17 00:00:00 2001 From: snider Date: Wed, 31 Dec 2025 01:55:24 +0000 Subject: [PATCH] fix: Address race conditions and network blocking issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: - Release mutex before HTTP calls in GetStats() to prevent blocking - Fix m.cmd race between Stop() and Wait() goroutine by capturing locally - Add context support to GetStats() for proper request cancellation High priority fixes: - Add existence check in collectMinerStats() before operating on miners - Add mutex-protected httpClient getter/setter for thread-safe test mocking Changes: - Miner interface now requires context.Context for GetStats() - Stats HTTP requests timeout after 5 seconds (was 30s client default) - All callers updated to pass context (service uses request context) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/mining/cmd/serve.go | 2 +- cmd/mining/cmd/status.go | 3 ++- pkg/mining/dual_mining_test.go | 3 ++- pkg/mining/manager.go | 16 +++++++++++++--- pkg/mining/miner.go | 28 ++++++++++++++-------------- pkg/mining/mining.go | 3 ++- pkg/mining/service.go | 2 +- pkg/mining/throttle_test.go | 7 ++++--- pkg/mining/ttminer.go | 2 +- pkg/mining/ttminer_start.go | 11 ++++++++--- pkg/mining/ttminer_stats.go | 30 +++++++++++++++++++++++------- pkg/mining/xmrig.go | 22 +++++++++++++++++++--- pkg/mining/xmrig_start.go | 11 ++++++++--- pkg/mining/xmrig_stats.go | 33 +++++++++++++++++++++++++++------ pkg/mining/xmrig_test.go | 27 ++++++++++++++------------- 15 files changed, 139 insertions(+), 61 deletions(-) diff --git a/cmd/mining/cmd/serve.go b/cmd/mining/cmd/serve.go index d1f3a86..14a5fda 100644 --- a/cmd/mining/cmd/serve.go +++ b/cmd/mining/cmd/serve.go @@ -119,7 +119,7 @@ var serveCmd = &cobra.Command{ if err != nil { fmt.Fprintf(os.Stderr, "Error getting miner status: %v\n", err) } else { - stats, err := miner.GetStats() + stats, err := miner.GetStats(context.Background()) if err != nil { fmt.Fprintf(os.Stderr, "Error getting miner stats: %v\n", err) } else { diff --git a/cmd/mining/cmd/status.go b/cmd/mining/cmd/status.go index abea9d8..02e4273 100644 --- a/cmd/mining/cmd/status.go +++ b/cmd/mining/cmd/status.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "github.com/spf13/cobra" @@ -23,7 +24,7 @@ var statusCmd = &cobra.Command{ return fmt.Errorf("failed to get miner: %w", err) } - stats, err := miner.GetStats() + stats, err := miner.GetStats(context.Background()) if err != nil { return fmt.Errorf("failed to get miner stats: %w", err) } diff --git a/pkg/mining/dual_mining_test.go b/pkg/mining/dual_mining_test.go index 80e4096..48789ad 100644 --- a/pkg/mining/dual_mining_test.go +++ b/pkg/mining/dual_mining_test.go @@ -1,6 +1,7 @@ package mining import ( + "context" "testing" "time" ) @@ -46,7 +47,7 @@ func TestDualMiningCPUAndGPU(t *testing.T) { time.Sleep(20 * time.Second) // Get stats - stats, err := minerInstance.GetStats() + stats, err := minerInstance.GetStats(context.Background()) if err != nil { t.Logf("Warning: couldn't get stats: %v", err) } else { diff --git a/pkg/mining/manager.go b/pkg/mining/manager.go index 5553980..c4551f3 100644 --- a/pkg/mining/manager.go +++ b/pkg/mining/manager.go @@ -1,6 +1,7 @@ package mining import ( + "context" "fmt" "log" "net" @@ -443,9 +444,19 @@ func (m *Manager) collectMinerStats() { now := time.Now() for _, miner := range minersToCollect { - stats, err := miner.GetStats() + minerName := miner.GetName() + + // Verify miner still exists before collecting stats + m.mu.RLock() + _, stillExists := m.miners[minerName] + m.mu.RUnlock() + if !stillExists { + continue // Miner was removed, skip it + } + + stats, err := miner.GetStats(context.Background()) if err != nil { - log.Printf("Error getting stats for miner %s: %v\n", miner.GetName(), err) + log.Printf("Error getting stats for miner %s: %v\n", minerName, err) continue } @@ -460,7 +471,6 @@ func (m *Manager) collectMinerStats() { // Persist to database if enabled if m.dbEnabled { - minerName := miner.GetName() minerType := minerTypes[minerName] dbPoint := database.HashratePoint{ Timestamp: point.Timestamp, diff --git a/pkg/mining/miner.go b/pkg/mining/miner.go index 08ffa5b..62c8933 100644 --- a/pkg/mining/miner.go +++ b/pkg/mining/miner.go @@ -132,9 +132,9 @@ func (b *BaseMiner) GetBinaryPath() string { // It first tries SIGTERM to allow cleanup, then SIGKILL if needed. func (b *BaseMiner) Stop() error { b.mu.Lock() - defer b.mu.Unlock() if !b.Running || b.cmd == nil { + b.mu.Unlock() return errors.New("miner is not running") } @@ -144,25 +144,27 @@ func (b *BaseMiner) Stop() error { b.stdinPipe = nil } - // Helper to clean up state - cleanup := func() { - b.Running = false - b.cmd = nil - } + // Capture cmd locally to avoid race with Wait() goroutine + cmd := b.cmd + process := cmd.Process + + // Mark as not running immediately to prevent concurrent Stop() calls + b.Running = false + b.cmd = nil + b.mu.Unlock() // Try graceful shutdown with SIGTERM first (Unix only) if runtime.GOOS != "windows" { - if err := b.cmd.Process.Signal(syscall.SIGTERM); err == nil { + if err := process.Signal(syscall.SIGTERM); err == nil { // Wait up to 3 seconds for graceful shutdown done := make(chan struct{}) go func() { - b.cmd.Process.Wait() + process.Wait() close(done) }() select { case <-done: - cleanup() return nil case <-time.After(3 * time.Second): // Process didn't exit gracefully, force kill below @@ -171,14 +173,12 @@ func (b *BaseMiner) Stop() error { } // Force kill and wait for process to exit - if err := b.cmd.Process.Kill(); err != nil { - cleanup() + if err := process.Kill(); err != nil { return err } // Wait for process to fully terminate to avoid zombies - b.cmd.Process.Wait() - cleanup() + process.Wait() return nil } @@ -214,7 +214,7 @@ func (b *BaseMiner) InstallFromURL(url string) error { defer os.Remove(tmpfile.Name()) defer tmpfile.Close() - resp, err := httpClient.Get(url) + resp, err := getHTTPClient().Get(url) if err != nil { return err } diff --git a/pkg/mining/mining.go b/pkg/mining/mining.go index 92edc04..eda05c4 100644 --- a/pkg/mining/mining.go +++ b/pkg/mining/mining.go @@ -1,6 +1,7 @@ package mining import ( + "context" "time" ) @@ -17,7 +18,7 @@ type Miner interface { Uninstall() error Start(config *Config) error Stop() error - GetStats() (*PerformanceMetrics, error) + GetStats(ctx context.Context) (*PerformanceMetrics, error) GetName() string GetPath() string GetBinaryPath() string diff --git a/pkg/mining/service.go b/pkg/mining/service.go index fafe1d4..1e2222e 100644 --- a/pkg/mining/service.go +++ b/pkg/mining/service.go @@ -483,7 +483,7 @@ func (s *Service) handleGetMinerStats(c *gin.Context) { c.JSON(http.StatusNotFound, gin.H{"error": "miner not found"}) return } - stats, err := miner.GetStats() + stats, err := miner.GetStats(c.Request.Context()) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return diff --git a/pkg/mining/throttle_test.go b/pkg/mining/throttle_test.go index cc475cb..7d67185 100644 --- a/pkg/mining/throttle_test.go +++ b/pkg/mining/throttle_test.go @@ -1,6 +1,7 @@ package mining import ( + "context" "runtime" "testing" "time" @@ -201,7 +202,7 @@ func TestMinerResourceIsolation(t *testing.T) { time.Sleep(10 * time.Second) // Get baseline hashrate for miner 1 alone - stats1Alone, err := miner1.GetStats() + stats1Alone, err := miner1.GetStats(context.Background()) if err != nil { t.Logf("Warning: couldn't get stats for miner 1: %v", err) } @@ -226,11 +227,11 @@ func TestMinerResourceIsolation(t *testing.T) { time.Sleep(15 * time.Second) // Check both miners are running and producing hashrate - stats1, err := miner1.GetStats() + stats1, err := miner1.GetStats(context.Background()) if err != nil { t.Logf("Warning: couldn't get stats for miner 1: %v", err) } - stats2, err := miner2.GetStats() + stats2, err := miner2.GetStats(context.Background()) if err != nil { t.Logf("Warning: couldn't get stats for miner 2: %v", err) } diff --git a/pkg/mining/ttminer.go b/pkg/mining/ttminer.go index 8de996e..5269a5e 100644 --- a/pkg/mining/ttminer.go +++ b/pkg/mining/ttminer.go @@ -85,7 +85,7 @@ func getTTMinerConfigPath() (string, error) { // GetLatestVersion fetches the latest version of TT-Miner from the GitHub API. func (m *TTMiner) GetLatestVersion() (string, error) { - resp, err := httpClient.Get("https://api.github.com/repos/TrailingStop/TT-Miner-release/releases/latest") + resp, err := getHTTPClient().Get("https://api.github.com/repos/TrailingStop/TT-Miner-release/releases/latest") if err != nil { return "", err } diff --git a/pkg/mining/ttminer_start.go b/pkg/mining/ttminer_start.go index 3416da2..f39887c 100644 --- a/pkg/mining/ttminer_start.go +++ b/pkg/mining/ttminer_start.go @@ -64,11 +64,16 @@ func (m *TTMiner) Start(config *Config) error { m.Running = true - // Monitor the process in a goroutine + // Capture cmd locally to avoid race with Stop() + cmd := m.cmd go func() { - err := m.cmd.Wait() + err := cmd.Wait() m.mu.Lock() - m.Running = false + // Only clear if this is still the same command (not restarted) + if m.cmd == cmd { + m.Running = false + m.cmd = nil + } m.mu.Unlock() if err != nil { log.Printf("TT-Miner exited with error: %v", err) diff --git a/pkg/mining/ttminer_stats.go b/pkg/mining/ttminer_stats.go index 2d2eba3..6fc863d 100644 --- a/pkg/mining/ttminer_stats.go +++ b/pkg/mining/ttminer_stats.go @@ -1,6 +1,7 @@ package mining import ( + "context" "encoding/json" "errors" "fmt" @@ -8,19 +9,32 @@ import ( ) // GetStats retrieves performance metrics from the TT-Miner API. -func (m *TTMiner) GetStats() (*PerformanceMetrics, error) { - m.mu.Lock() - defer m.mu.Unlock() - +func (m *TTMiner) GetStats(ctx context.Context) (*PerformanceMetrics, error) { + // Read state under RLock, then release before HTTP call + m.mu.RLock() if !m.Running { + m.mu.RUnlock() return nil, errors.New("miner is not running") } if m.API == nil || m.API.ListenPort == 0 { + m.mu.RUnlock() return nil, errors.New("miner API not configured or port is zero") } + host := m.API.ListenHost + port := m.API.ListenPort + m.mu.RUnlock() - // TT-Miner API endpoint - try the summary endpoint - resp, err := httpClient.Get(fmt.Sprintf("http://%s:%d/summary", m.API.ListenHost, m.API.ListenPort)) + // Create request with context and timeout + reqCtx, cancel := context.WithTimeout(ctx, statsTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, "GET", fmt.Sprintf("http://%s:%d/summary", host, port), nil) + if err != nil { + return nil, err + } + + // HTTP call outside the lock to avoid blocking other operations + resp, err := getHTTPClient().Do(req) if err != nil { return nil, err } @@ -35,8 +49,10 @@ func (m *TTMiner) GetStats() (*PerformanceMetrics, error) { return nil, err } - // Store the full summary in the miner struct + // Store the full summary in the miner struct (requires lock) + m.mu.Lock() m.FullStats = &summary + m.mu.Unlock() // Calculate total hashrate from all GPUs var totalHashrate float64 diff --git a/pkg/mining/xmrig.go b/pkg/mining/xmrig.go index eb8fbea..dd56195 100644 --- a/pkg/mining/xmrig.go +++ b/pkg/mining/xmrig.go @@ -11,6 +11,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "time" "github.com/adrg/xdg" @@ -22,8 +23,23 @@ type XMRigMiner struct { FullStats *XMRigSummary `json:"full_stats,omitempty"` } -var httpClient = &http.Client{ - Timeout: 30 * time.Second, +var ( + httpClient = &http.Client{Timeout: 30 * time.Second} + httpClientMu sync.RWMutex +) + +// getHTTPClient returns the HTTP client with proper synchronization +func getHTTPClient() *http.Client { + httpClientMu.RLock() + defer httpClientMu.RUnlock() + return httpClient +} + +// setHTTPClient sets the HTTP client (for testing) +func setHTTPClient(client *http.Client) { + httpClientMu.Lock() + defer httpClientMu.Unlock() + httpClient = client } // NewXMRigMiner creates a new XMRig miner instance with default settings. @@ -70,7 +86,7 @@ var getXMRigConfigPath = func(instanceName string) (string, error) { // GetLatestVersion fetches the latest version of XMRig from the GitHub API. func (m *XMRigMiner) GetLatestVersion() (string, error) { - resp, err := httpClient.Get("https://api.github.com/repos/xmrig/xmrig/releases/latest") + resp, err := getHTTPClient().Get("https://api.github.com/repos/xmrig/xmrig/releases/latest") if err != nil { return "", err } diff --git a/pkg/mining/xmrig_start.go b/pkg/mining/xmrig_start.go index e11febf..1e07067 100644 --- a/pkg/mining/xmrig_start.go +++ b/pkg/mining/xmrig_start.go @@ -87,11 +87,16 @@ func (m *XMRigMiner) Start(config *Config) error { m.Running = true + // Capture cmd locally to avoid race with Stop() + cmd := m.cmd go func() { - m.cmd.Wait() + cmd.Wait() m.mu.Lock() - m.Running = false - m.cmd = nil + // Only clear if this is still the same command (not restarted) + if m.cmd == cmd { + m.Running = false + m.cmd = nil + } m.mu.Unlock() }() diff --git a/pkg/mining/xmrig_stats.go b/pkg/mining/xmrig_stats.go index f2fe6e7..9e9d2a1 100644 --- a/pkg/mining/xmrig_stats.go +++ b/pkg/mining/xmrig_stats.go @@ -1,25 +1,44 @@ package mining import ( + "context" "encoding/json" "errors" "fmt" "net/http" + "time" ) -// GetStats retrieves the performance statistics from the running XMRig miner. -func (m *XMRigMiner) GetStats() (*PerformanceMetrics, error) { - m.mu.Lock() - defer m.mu.Unlock() +// statsTimeout is the timeout for stats HTTP requests (shorter than general timeout) +const statsTimeout = 5 * time.Second +// GetStats retrieves the performance statistics from the running XMRig miner. +func (m *XMRigMiner) GetStats(ctx context.Context) (*PerformanceMetrics, error) { + // Read state under RLock, then release before HTTP call + m.mu.RLock() if !m.Running { + m.mu.RUnlock() return nil, errors.New("miner is not running") } if m.API == nil || m.API.ListenPort == 0 { + m.mu.RUnlock() return nil, errors.New("miner API not configured or port is zero") } + host := m.API.ListenHost + port := m.API.ListenPort + m.mu.RUnlock() - resp, err := httpClient.Get(fmt.Sprintf("http://%s:%d/2/summary", m.API.ListenHost, m.API.ListenPort)) + // Create request with context and timeout + reqCtx, cancel := context.WithTimeout(ctx, statsTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, "GET", fmt.Sprintf("http://%s:%d/2/summary", host, port), nil) + if err != nil { + return nil, err + } + + // HTTP call outside the lock to avoid blocking other operations + resp, err := getHTTPClient().Do(req) if err != nil { return nil, err } @@ -34,8 +53,10 @@ func (m *XMRigMiner) GetStats() (*PerformanceMetrics, error) { return nil, err } - // Store the full summary in the miner struct + // Store the full summary in the miner struct (requires lock) + m.mu.Lock() m.FullStats = &summary + m.mu.Unlock() var hashrate int if len(summary.Hashrate.Total) > 0 { diff --git a/pkg/mining/xmrig_test.go b/pkg/mining/xmrig_test.go index 0c4ba05..8504627 100644 --- a/pkg/mining/xmrig_test.go +++ b/pkg/mining/xmrig_test.go @@ -1,6 +1,7 @@ package mining import ( + "context" "encoding/json" "fmt" "io" @@ -63,8 +64,8 @@ func TestXMRigMiner_GetName_Good(t *testing.T) { } func TestXMRigMiner_GetLatestVersion_Good(t *testing.T) { - originalClient := httpClient - httpClient = newTestClient(func(req *http.Request) *http.Response { + originalClient := getHTTPClient() + setHTTPClient(newTestClient(func(req *http.Request) *http.Response { if req.URL.String() != "https://api.github.com/repos/xmrig/xmrig/releases/latest" { return &http.Response{ StatusCode: http.StatusNotFound, @@ -77,8 +78,8 @@ func TestXMRigMiner_GetLatestVersion_Good(t *testing.T) { Body: io.NopCloser(strings.NewReader(`{"tag_name": "v6.18.0"}`)), Header: make(http.Header), } - }) - defer func() { httpClient = originalClient }() + })) + defer setHTTPClient(originalClient) miner := NewXMRigMiner() version, err := miner.GetLatestVersion() @@ -91,15 +92,15 @@ func TestXMRigMiner_GetLatestVersion_Good(t *testing.T) { } func TestXMRigMiner_GetLatestVersion_Bad(t *testing.T) { - originalClient := httpClient - httpClient = newTestClient(func(req *http.Request) *http.Response { + originalClient := getHTTPClient() + setHTTPClient(newTestClient(func(req *http.Request) *http.Response { return &http.Response{ StatusCode: http.StatusNotFound, Body: io.NopCloser(strings.NewReader("Not Found")), Header: make(http.Header), } - }) - defer func() { httpClient = originalClient }() + })) + defer setHTTPClient(originalClient) miner := NewXMRigMiner() _, err := miner.GetLatestVersion() @@ -197,9 +198,9 @@ func TestXMRigMiner_GetStats_Good(t *testing.T) { })) defer server.Close() - originalHTTPClient := httpClient - httpClient = server.Client() - defer func() { httpClient = originalHTTPClient }() + originalHTTPClient := getHTTPClient() + setHTTPClient(server.Client()) + defer setHTTPClient(originalHTTPClient) miner := NewXMRigMiner() miner.Running = true // Mock running state @@ -209,7 +210,7 @@ func TestXMRigMiner_GetStats_Good(t *testing.T) { miner.API.ListenHost = parts[0] fmt.Sscanf(parts[1], "%d", &miner.API.ListenPort) - stats, err := miner.GetStats() + stats, err := miner.GetStats(context.Background()) if err != nil { t.Fatalf("GetStats() returned an error: %v", err) } @@ -237,7 +238,7 @@ func TestXMRigMiner_GetStats_Bad(t *testing.T) { miner.API.ListenHost = "127.0.0.1" miner.API.ListenPort = 9999 // A port that is unlikely to be in use - _, err := miner.GetStats() + _, err := miner.GetStats(context.Background()) if err == nil { t.Fatalf("GetStats() did not return an error") }