Race condition on disableTiming field closes #4808 (rebased) (#4890)

* Add test for #4808: Race condition on disableTiming field

Add test that verifies disableTiming field uses atomic.Bool instead of
plain bool to prevent race conditions.

The test checks that:
- disableTiming is declared as atomic.Bool
- shouldFetchTiming() uses atomic Load()
- getQueryTiming() uses atomic Store() operations
- Direct assignments that cause races are not present

This test will fail until the implementation is fixed.

* Fix #4808: Use atomic.Bool for disableTiming to prevent race condition

pkg/db/db_client/db_client.go

@@ -6,6 +6,7 @@ import (
 	"log"
 	"strings"
 	"sync"
+	"sync/atomic"
 
 	"github.com/jackc/pgx/v5/pgconn"
 	"github.com/jackc/pgx/v5/pgxpool"
@@ -56,7 +57,7 @@ type DbClient struct {
 	// the default user search path
 	userSearchPath []string
 	// disable timing - set whilst in process of querying the timing
-	disableTiming        bool
+	disableTiming        atomic.Bool
 	onConnectionCallback DbConnectionCallback
 }
 
@@ -135,7 +136,7 @@ func (c *DbClient) loadServerSettings(ctx context.Context) error {
 
 func (c *DbClient) shouldFetchTiming() bool {
 	// check for override flag (this is to prevent timing being fetched when we read the timing metadata table)
-	if c.disableTiming {
+	if c.disableTiming.Load() {
 		return false
 	}
 	// only fetch timing if timing flag is set, or output is JSON

pkg/db/db_client/db_client_execute.go

@@ -187,11 +187,11 @@ func (c *DbClient) getQueryTiming(ctx context.Context, startTime time.Time, sess
 		DurationMs: time.Since(startTime).Milliseconds(),
 	}
 	// disable fetching timing information to avoid recursion
-	c.disableTiming = true
+	c.disableTiming.Store(true)
 
 	// whatever happens, we need to reenable timing, and send the result back with at least the duration
 	defer func() {
-		c.disableTiming = false
+		c.disableTiming.Store(false)
 		resultChannel.SetTiming(timingResult)
 	}()
 

pkg/db/db_client/db_client_test.go

@@ -268,3 +268,52 @@ func TestDbClient_BeforeCloseCallbackNilSafety(t *testing.T) {
 	assert.Contains(t, sourceCode, "conn.PgConn() != nil",
 		"BeforeClose should check if PgConn() is nil")
 }
+
+// TestDbClient_DisableTimingFlag tests for race conditions on the disableTiming field
+// Reference: https://github.com/turbot/steampipe/issues/4808
+//
+// This test demonstrates that the disableTiming boolean is accessed from multiple
+// goroutines without synchronization, which can cause data races.
+//
+// The race occurs between:
+// - shouldFetchTiming() reading disableTiming (db_client.go:138)
+// - getQueryTiming() writing disableTiming (db_client_execute.go:190, 194)
+func TestDbClient_DisableTimingFlag(t *testing.T) {
+	// Read the db_client.go file to check the field type
+	content, err := os.ReadFile("db_client.go")
+	require.NoError(t, err, "should be able to read db_client.go")
+
+	sourceCode := string(content)
+
+	// Verify that disableTiming uses atomic.Bool instead of plain bool
+	// The field declaration should be: disableTiming atomic.Bool
+	assert.Contains(t, sourceCode, "disableTiming        atomic.Bool",
+		"disableTiming must use atomic.Bool to prevent race conditions")
+
+	// Verify the atomic import exists
+	assert.Contains(t, sourceCode, "\"sync/atomic\"",
+		"sync/atomic package must be imported for atomic.Bool")
+
+	// Check that db_client_execute.go uses atomic operations
+	executeContent, err := os.ReadFile("db_client_execute.go")
+	require.NoError(t, err, "should be able to read db_client_execute.go")
+
+	executeCode := string(executeContent)
+
+	// Verify atomic Store operations are used instead of direct assignment
+	assert.Contains(t, executeCode, ".Store(true)",
+		"disableTiming writes must use atomic Store(true)")
+	assert.Contains(t, executeCode, ".Store(false)",
+		"disableTiming writes must use atomic Store(false)")
+
+	// The old non-atomic assignments should not be present
+	assert.NotContains(t, executeCode, "c.disableTiming = true",
+		"direct assignment to disableTiming creates race condition")
+	assert.NotContains(t, executeCode, "c.disableTiming = false",
+		"direct assignment to disableTiming creates race condition")
+
+	// Verify that shouldFetchTiming uses atomic Load
+	shouldFetchTimingLine := "if c.disableTiming.Load() {"
+	assert.Contains(t, sourceCode, shouldFetchTimingLine,
+		"disableTiming reads must use atomic Load()")
+}