[Bug]: Fatal Concurrency Gridlock - Mutex Write-Lock Held Over Synchronous Network I/O in Circuit Breaker Recovery

[adityakrmishra]

Bug Overview

During a deep architectural audit of the framework's HTTP service layer, I identified a critical flaw in the Circuit Breaker recovery mechanism (pkg/gofr/service/circuit_breaker.go).

The tryCircuitRecovery() method acquires a global sync.RWMutex write-lock (cb.mu.Lock()) and performs a synchronous, blocking HTTP network call (cb.healthCheck()) without releasing the lock.

Ironically, the comment in executeWithCircuitBreaker() explicitly states // Circuit is open - try recovery without holding lock, but the implementation inside tryCircuitRecovery() immediately violates this.

Impact Analysis

In a production microservices environment, this flaw leads to a catastrophic cascading failure.

When a downstream dependency fails and the circuit breaker trips into the OpenState, the next request after the interval will attempt recovery. During the entirety of the cb.healthCheck() network operation, every single concurrent request targeting this service blocks indefinitely at cb.mu.RLock() in executeWithCircuitBreaker.

If the downstream service is unresponsive or slow (e.g., a tarpit scenario), this instantly exhausts the server's goroutine pool and incoming HTTP connection limits. The entire GoFr application will freeze, stopping all traffic processing and effectively self-inflicting a massive Denial of Service.

Root Cause Code

File: pkg/gofr/service/circuit_breaker.go

The critical failure resides inside the tryCircuitRecovery() method:

func (cb *circuitBreaker) tryCircuitRecovery() bool {
	cb.mu.Lock()           // <--- FATAL FLAW: Global Write-lock acquired
	defer cb.mu.Unlock()

	if cb.state == ClosedState {
		return true
	}

	if time.Since(cb.lastChecked) > cb.interval {
		cb.lastChecked = time.Now()
		
		// FATAL FLAW: Blocking network I/O executed while holding the global write-lock!
		if cb.healthCheck(context.TODO()) {
			cb.resetCircuit()
			return true
		}
	}

	return false
}

Proof of Concept

• Initialize a GoFr application with an HTTP client configured with a circuit breaker (Threshold: 2, Interval: 5s).
• Trigger the circuit breaker into OpenState by sending requests that return 500-level errors.
• Wait for the 5-second Interval to expire.
• Configure the downstream server's health check endpoint to act as a tarpit (accept the connection but intentionally delay the response for 15+ seconds).
• Send a single request to the GoFr application. It will enter tryCircuitRecovery(), acquire cb.mu.Lock(), and hang waiting for the network I/O.
• Concurrently blast the GoFr application with high-throughput traffic using a load tester (e.g., hey -c 200 -z 10s).
• Result: Observe that all concurrent requests instantly hang waiting for cb.mu.RLock(). The GoFr application drops to 0 RPS and becomes completely unresponsive.

Proposed Architectural Fix

We must completely decouple the network I/O from the mutex.
The tryCircuitRecovery method should only hold the write-lock briefly to check the time interval and update the lastChecked timestamp.

If a health check is required, the lock must be released before invoking cb.healthCheck(ctx). Once the health check returns asynchronously, a secondary lock acquisition can safely update the state to ClosedState via cb.resetCircuit(). This ensures the mutex is only used for nanosecond-level in-memory state mutations, guaranteeing non-blocking throughput.

I can open a PR for this immediately if the maintainers approve of the proposed architecture!