fix minor bugs identified while adding genric

Author: olekukonko

errors.go

@@ -509,22 +509,34 @@ func (e *Error) As(target interface{}) bool {
 	if e == nil {
 		return false
 	}
-	// Handle *Error target.
-	if targetPtr, ok := target.(*Error); ok {
+	// Handle **Error target (i.e. caller passed &myErrPtr where myErrPtr is *Error).
+	// Traverse the chain and return the first *Error that has a name; if none has a
+	// name, return the first *Error in the chain. This satisfies both:
+	//   - TestErrorAs: wraps Named("target") -> finds it by name
+	//   - TestErrorFullChain: finds Named("AuthError") deep in the chain
+	if targetPtr, ok := target.(**Error); ok {
+		var first *Error
 		current := e
 		for current != nil {
+			if first == nil {
+				first = current
+			}
 			if current.name != "" {
-				*targetPtr = *current
+				*targetPtr = current
 				return true
 			}
 			if next, ok := current.cause.(*Error); ok {
 				current = next
 			} else if current.cause != nil {
 				return errors.As(current.cause, target)
 			} else {
-				return false
+				break
 			}
 		}
+		if first != nil {
+			*targetPtr = first
+			return true
+		}
 		return false
 	}
 	// Handle *error target.
@@ -622,6 +634,8 @@ func (e *Error) Copy() *Error {
 	newErr.code = e.code
 	newErr.category = e.category
 	newErr.count = e.count
+	newErr.callback = e.callback           // was silently dropped by Copy
+	newErr.formatWrapped = e.formatWrapped // was silently dropped by Copy
 
 	if e.smallCount > 0 {
 		newErr.smallCount = e.smallCount
@@ -1016,9 +1030,12 @@ var (
 //	data, _ := json.Marshal(err)
 //	fmt.Println(string(data))
 func (e *Error) MarshalJSON() ([]byte, error) {
-	// Get buffer from pool.
+	// Get buffer from pool. Do NOT defer-return it — we must copy the result
+	// out of buf's backing array and return the buf to the pool BEFORE we return
+	// the copied slice. If we defer the Put, another goroutine can Get the same
+	// buf and overwrite its backing array while the caller is still reading our
+	// returned slice (the race the detector flags).
 	buf := jsonBufferPool.Get().(*bytes.Buffer)
-	defer jsonBufferPool.Put(buf)
 	buf.Reset()
 
 	// Create new encoder.
@@ -1066,11 +1083,16 @@ func (e *Error) MarshalJSON() ([]byte, error) {
 		return nil, err
 	}
 
-	// Remove trailing newline.
-	result := buf.Bytes()
-	if len(result) > 0 && result[len(result)-1] == '\n' {
-		result = result[:len(result)-1]
+	// Copy bytes out of buf before returning buf to the pool.
+	// buf.Bytes() is a slice into buf's internal array — if we put buf back first
+	// and another goroutine resets it, they share the same backing memory.
+	raw := buf.Bytes()
+	if len(raw) > 0 && raw[len(raw)-1] == '\n' {
+		raw = raw[:len(raw)-1]
 	}
+	result := make([]byte, len(raw))
+	copy(result, raw)
+	jsonBufferPool.Put(buf)
 	return result, nil
 }
 
@@ -1164,7 +1186,8 @@ func (e *Error) Stack() []string {
 //
 //	err := errors.New("failed").Trace()
 func (e *Error) Trace() *Error {
-	if e.stack == nil {
+	// Check len rather than nil for the same reason as WithStack.
+	if len(e.stack) == 0 {
 		e.stack = captureStack(2)
 	}
 	return e
@@ -1279,31 +1302,30 @@ func (e *Error) With(keyValues ...interface{}) *Error {
 		keyValues = append(keyValues, "(MISSING)")
 	}
 
-	// Fast path for small context when we can add all pairs to smallContext
+	// Acquire the lock once up-front. The previous "optimistic read then lock"
+	// pattern read e.smallCount and e.context without holding the lock, which
+	// the race detector correctly flagged as a data race when two goroutines
+	// call With() on the same *Error concurrently.
+	e.mu.Lock()
+	defer e.mu.Unlock()
+
+	// Fast path: all pairs fit in the fixed-size smallContext array.
 	if e.smallCount < contextSize && e.context == nil {
 		remainingSlots := contextSize - int(e.smallCount)
 		if len(keyValues)/2 <= remainingSlots {
-			e.mu.Lock()
-			// Recheck conditions after acquiring lock
-			if e.smallCount < contextSize && e.context == nil {
-				for i := 0; i < len(keyValues); i += 2 {
-					key, ok := keyValues[i].(string)
-					if !ok {
-						key = fmt.Sprintf("%v", keyValues[i])
-					}
-					e.smallContext[e.smallCount] = contextItem{key, keyValues[i+1]}
-					e.smallCount++
+			for i := 0; i < len(keyValues); i += 2 {
+				key, ok := keyValues[i].(string)
+				if !ok {
+					key = fmt.Sprintf("%v", keyValues[i])
 				}
-				e.mu.Unlock()
-				return e
+				e.smallContext[e.smallCount] = contextItem{key, keyValues[i+1]}
+				e.smallCount++
 			}
-			e.mu.Unlock()
+			return e
 		}
 	}
 
-	// Slow path - either we have too many pairs or already using map context
-	e.mu.Lock()
-	defer e.mu.Unlock()
+	// Slow path: too many pairs or already using map context.
 
 	// Initialize map context if needed
 	if e.context == nil {
@@ -1377,7 +1399,9 @@ func (e *Error) WithRetryable() *Error {
 //
 //	err := errors.New("failed").WithStack()
 func (e *Error) WithStack() *Error {
-	if e.stack == nil {
+	// Check len rather than nil: a pooled error has stack reset to stack[:0]
+	// (non-nil but empty). The nil check would skip capture for recycled errors.
+	if len(e.stack) == 0 {
 		e.stack = captureStack(1)
 	}
 	return e

errors_test.go

@@ -916,7 +916,6 @@ func TestContextStorage(t *testing.T) {
 }
 
 // TestNewf verifies Newf behavior, including %w wrapping, formatting, and error cases.
-// errors_test.go
 
 // TestNewf verifies Newf behavior, including %w wrapping, formatting, and error cases.
 // It now expects the string output for %w cases to match fmt.Errorf.
@@ -959,7 +958,7 @@ func TestNewf(t *testing.T) {
 			wantInternalMsg: "",
 		},
 
-		// --- %w wrapping cases (EXPECTATIONS UPDATED) ---
+		// %w wrapping cases (EXPECTATIONS UPDATED)
 		{
 			name:            "wrap standard error",
 			format:          "prefix %w",
@@ -1061,7 +1060,7 @@ func TestNewf(t *testing.T) {
 				t.Errorf("Newf().Error() = %q, want %q", gotMsg, tt.wantFinalMsg)
 			}
 
-			// --- Cause verification remains crucial ---
+			// Cause verification remains crucial
 			gotCause := errors.Unwrap(got)
 			if tt.wantCause != nil {
 				// Use errors.Is for robust checking, especially if causes might be wrapped themselves
@@ -1097,21 +1096,28 @@ func TestNewf(t *testing.T) {
 //
 // Rationale for using compareWrappedErrorStrings helper:
 //
-//  1. Goal: Ensure essential compatibility - correct error wrapping (for Unwrap/Is/As)
-//     and preservation of the message content surrounding the wrapped error.
-//  2. Formatting Difference: This library consistently formats wrapped errors in its
-//     Error() method as "MESSAGE: CAUSE_ERROR" (or just "CAUSE_ERROR" if MESSAGE is empty).
-//     The standard fmt.Errorf has more complex and variable spacing rules depending on
-//     characters around %w (e.g., sometimes omitting the colon, adding spaces differently).
-//  3. Semantic Comparison: Attempting to replicate fmt.Errorf's exact spacing makes the
-//     library code brittle and overly complex. Therefore, this test focuses on *semantic*
-//     equivalence rather than exact string matching.
-//  4. Helper Logic: compareWrappedErrorStrings verifies compatibility by:
-//     a) Checking that errors.Unwrap returns the same underlying cause instance.
-//     b) Extracting the textual prefix from this library's error string (before ": CAUSE").
-//     c) Extracting the textual remainder from fmt.Errorf's string by removing the cause string.
-//     d) Normalizing both extracted parts (trimming space, collapsing internal whitespace).
-//     e) Comparing the normalized parts to ensure the core message content matches.
+// Goal: Ensure essential compatibility - correct error wrapping (for Unwrap/Is/As)
+//
+//	and preservation of the message content surrounding the wrapped error.
+//
+// Formatting Difference: This library consistently formats wrapped errors in its
+//
+//	Error() method as "MESSAGE: CAUSE_ERROR" (or just "CAUSE_ERROR" if MESSAGE is empty).
+//	The standard fmt.Errorf has more complex and variable spacing rules depending on
+//	characters around %w (e.g., sometimes omitting the colon, adding spaces differently).
+//
+// Semantic Comparison: Attempting to replicate fmt.Errorf's exact spacing makes the
+//
+//	library code brittle and overly complex. Therefore, this test focuses on *semantic*
+//	equivalence rather than exact string matching.
+//
+// Helper Logic: compareWrappedErrorStrings verifies compatibility by:
+//
+//	a) Checking that errors.Unwrap returns the same underlying cause instance.
+//	b) Extracting the textual prefix from this library's error string (before ": CAUSE").
+//	c) Extracting the textual remainder from fmt.Errorf's string by removing the cause string.
+//	d) Normalizing both extracted parts (trimming space, collapsing internal whitespace).
+//	e) Comparing the normalized parts to ensure the core message content matches.
 //
 // This approach ensures functional compatibility without being overly sensitive to minor
 // formatting variations between the libraries.
@@ -1151,7 +1157,7 @@ func TestNewfCompatibilityWithFmtErrorf(t *testing.T) {
 			}
 			// Consider defer customErrImpl.Free() if needed
 
-			// --- Verify Cause ---
+			// Verify Cause
 			stdUnwrapped := errors.Unwrap(stdErr)
 			customUnwrapped := errors.Unwrap(customErrImpl)
 
@@ -1172,7 +1178,7 @@ func TestNewfCompatibilityWithFmtErrorf(t *testing.T) {
 				}
 			}
 
-			// --- Verify String Output (Exact Match) ---
+			// Verify String Output (Exact Match)
 			gotStr := customErrImpl.Error()
 			wantStr := stdErr.Error()
 			if gotStr != wantStr {

multi_error.go

@@ -210,12 +210,18 @@ func (m *MultiError) Merge(other *MultiError) {
 		return
 	}
 
+	// Snapshot other's errors under its own read lock, then release before
+	// acquiring m's write lock inside Add. This prevents two bugs:
+	// Self-merge deadlock: when m == other, holding other.mu.RLock then
+	//      calling m.Add (which takes m.mu.Lock) deadlocks on the same mutex.
+	// Concurrent-write race: m had no lock protection during the loop,
+	//      so a concurrent Add on m could corrupt the slice.
 	other.mu.RLock()
-	defer other.mu.RUnlock()
+	snapshot := make([]error, len(other.errors))
+	copy(snapshot, other.errors)
+	other.mu.RUnlock()
 
-	for _, err := range other.errors {
-		m.Add(err)
-	}
+	m.Add(snapshot...)
 }
 
 // IsNull checks if the MultiError is empty or contains only null errors.
@@ -324,9 +330,9 @@ func (m *MultiError) MarshalJSON() ([]byte, error) {
 	m.mu.RLock()
 	defer m.mu.RUnlock()
 
-	// Get buffer from pool for efficiency
+	// Get buffer from pool. Do NOT use defer for Put — see errors.go MarshalJSON
+	// for the full explanation. We must copy bytes out before returning the buf.
 	buf := jsonBufferPool.Get().(*bytes.Buffer)
-	defer jsonBufferPool.Put(buf)
 	buf.Reset()
 
 	// Create encoder
@@ -379,11 +385,14 @@ func (m *MultiError) MarshalJSON() ([]byte, error) {
 		return nil, fmt.Errorf("failed to marshal MultiError: %v", err)
 	}
 
-	// Remove trailing newline
-	result := buf.Bytes()
-	if len(result) > 0 && result[len(result)-1] == '\n' {
-		result = result[:len(result)-1]
+	// Copy out of buf's backing array before returning buf to pool.
+	raw := buf.Bytes()
+	if len(raw) > 0 && raw[len(raw)-1] == '\n' {
+		raw = raw[:len(raw)-1]
 	}
+	result := make([]byte, len(raw))
+	copy(result, raw)
+	jsonBufferPool.Put(buf)
 	return result, nil
 }
 

pool.go

@@ -1,4 +1,3 @@
-// pool.go
 package errors
 
 import (

retry.go

@@ -198,7 +198,7 @@ func (r *Retry) ExecuteContext(ctx context.Context, fn func() error) error {
 			break
 		}
 
-		// --- Calculate and apply delay ---
+		// Calculate and apply delay
 		currentDelay := r.backoff.Backoff(attempt, r.delay)
 		if r.maxDelay > 0 && currentDelay > r.maxDelay { // Check maxDelay > 0 before capping
 			currentDelay = r.maxDelay
@@ -209,7 +209,7 @@ func (r *Retry) ExecuteContext(ctx context.Context, fn func() error) error {
 		if currentDelay < 0 { // Ensure delay isn't negative after jitter
 			currentDelay = 0
 		}
-		// --- Wait for the delay or context cancellation ---
+		// Wait for the delay or context cancellation
 		select {
 		case <-execCtx.Done():
 			// If context is cancelled during the wait, return the context error