BED-5717 Fix Resetting Connections #148
Conversation
…e connection. Added info line
WalkthroughBHE client now stores provided Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant BHEClient
note over Caller,BHEClient: Initialization
Caller->>BHEClient: NewBHEClient(host, token, tokenId, ...)
activate BHEClient
BHEClient->>BHEClient: Assign token, tokenId fields
BHEClient-->>Caller: Return configured client
deactivate BHEClient
sequenceDiagram
autonumber
participant BHEClient
participant HTTPClient as HTTP Client/Transport
note over BHEClient: Connection reset path
BHEClient->>BHEClient: Compute needsReset = currentRequestCount >= requestLimit
alt needsReset true
Note right of BHEClient #D6F5D6: V(1) log emitted ("Max requests per connection limit reached...")
BHEClient->>HTTPClient: CloseIdleConnections()
BHEClient->>BHEClient: Replace HTTP client instance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/bloodhound/client.go (1)
419-432: Data race on currentRequestCount; move threshold check under the lock
currentRequestCountis incremented unders.mubut read outside the lock, which is a data race and can trigger multiple concurrent resets. Move the check under the lock and compute a localneedResetflag. CallresetConnection()after unlocking to avoid deadlock (it locks internally).Apply this diff:
func (s *BHEClient) incrementRequest() error { - s.mu.Lock() - s.currentRequestCount += 1 - s.mu.Unlock() - - if s.currentRequestCount >= s.requestLimit { + s.mu.Lock() + s.currentRequestCount++ + needReset := s.currentRequestCount >= s.requestLimit + s.mu.Unlock() + + if needReset { if err := s.resetConnection(); err != nil { s.log.Error(err, "error resetting BHE http client connection") return err } } return nil }
🧹 Nitpick comments (2)
client/bloodhound/client.go (2)
406-407: Enhance the new reset log with actionable context (limit value)Including the limit in the log helps correlate resets with configured thresholds during debugging.
Apply this diff:
- s.log.V(1).Info("Max requests per connection limit reached, resetting connection with BHE server") + s.log.V(1).Info( + "Max requests per connection limit reached, resetting connection with BHE server", + "requestLimit", s.requestLimit, +)
111-113: Minor log typo: “from from”Duplicate word in the GOAWAY error logs.
Apply this diff:
- s.log.Error(err, fmt.Sprintf("received GOAWAY from from AWS load balancer while requesting %s; attempt %d/%d; trying again", req.URL, currentAttempt+1, s.maxRetries)) + s.log.Error(err, fmt.Sprintf("received GOAWAY from AWS load balancer while requesting %s; attempt %d/%d; trying again", req.URL, currentAttempt+1, s.maxRetries))Repeat the same change for the identical message in Ingest.
Also applies to: 207-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/bloodhound/client.go(2 hunks)
🔇 Additional comments (1)
client/bloodhound/client.go (1)
82-84: Good fix: persist token and tokenId on the clientStoring
tokenandtokenIdin theBHEClientensuresresetConnection()can rebuild a correctly signed transport. This addresses the bug where reconnections lost auth context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
client/bloodhound/client_test.go (6)
87-88: Derive the WaitGroup count from maxRetries instead of hardcoding 2.Future-proof the test so it tracks changes to retry policy without silently under/over-counting.
Apply this diff:
- wg := sync.WaitGroup{} - wg.Add(2) + wg := sync.WaitGroup{} + // initial attempt + retries + wg.Add(maxRetries + 1)
90-94: Guarantee wg.Done is always called in the handler.Use defer to avoid hangs if the handler ever early-returns or panics in future edits.
- testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requestCount++ - w.WriteHeader(http.StatusGatewayTimeout) - wg.Done() - })) + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer wg.Done() + requestCount++ + w.WriteHeader(http.StatusGatewayTimeout) + }))
110-110: Avoid indefinite waits by guarding wg.Wait with a timeout.If the retry loop behavior changes or a Done is missed, this test could hang. Add a timeout wrapper.
Apply this diff and add a time import if you adopt it:
- wg.Wait() + done := make(chan struct{}) + go func() { defer close(done); wg.Wait() }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for retries to complete") + }If you adopt the above, add to the imports:
- time
66-71: Close the httptest server to prevent resource leaks.Add a defer to ensure proper cleanup in all paths.
testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusAccepted) })) + defer testServer.Close()
90-98: Close the httptest server in the failure/retry case as well.Same rationale—avoid leaking listeners between subtests.
- testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer wg.Done() requestCount++ w.WriteHeader(http.StatusGatewayTimeout) })) + defer testServer.Close()
43-47: Also close the server in TestBHEClient_SendRequest retry test.Consistent cleanup across tests keeps CI stable and avoids port exhaustion on flaky runs.
testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requestCount++ w.WriteHeader(http.StatusInternalServerError) })) + defer testServer.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/bloodhound/client_test.go(3 hunks)
🔇 Additional comments (1)
client/bloodhound/client_test.go (1)
8-8: Importing sync for WaitGroup is appropriate.This aligns with the added synchronization in the "retry after failures" ingest test.
2 participants
A bug was found when resetting connections due to improperly storing the
tokenidandtokenof the BHE clientThis also adds an additional information log when the client resets a connection with the BHE server
Tested locally by setting the
maxreqsperconnsettings to a low number (50) to replicate the issue and ensured AzureHound could re-connect to thetestinstance after the connection was resetSummary by CodeRabbit