fix(remoting): clean up pendingResponses on error paths to prevent map leak#3440
Open
Aias00 wants to merge 3 commits into
Open
fix(remoting): clean up pendingResponses on error paths to prevent map leak#3440Aias00 wants to merge 3 commits into
Aias00 wants to merge 3 commits into
Conversation
…p leak The global pendingResponses sync.Map is cleaned up on the success path via Response.Handle() -> removePendingResponse(), but entries were never removed when requests fail: - ExchangeClient.Request(): AddPendingResponse called before client.Request(), but on error the entry was left in the map - ExchangeClient.AsyncRequest(): same pattern, same leak - getty heartbeat(): timeout branch never called removePendingResponse, leaving the entry to accumulate indefinitely Add removePendingResponse calls in all three error/timeout paths. Export RemovePendingResponse so the getty package (external to remoting) can call it from its heartbeat timeout handler.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR ensures pending remoting responses are cleaned up when requests time out or fail, preventing stale entries from lingering in the pending response map.
Changes:
- Remove pending responses on heartbeat read timeout in the Getty listener.
- Remove pending responses when
ExchangeClientrequest/async-request fails immediately. - Export
RemovePendingResponseto allow subpackages (e.g.,remoting/getty) to trigger cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| remoting/getty/listener.go | Cleans up the pending response entry on heartbeat timeout. |
| remoting/exchange_client.go | Cleans up the pending response entry when request submission returns an error. |
| remoting/exchange.go | Adds an exported pending-response removal wrapper for cross-package use. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+187
to
+191
| // RemovePendingResponse removes and returns the pending response for the given sequence ID. | ||
| // It is the exported version of removePendingResponse for use by external packages. | ||
| func RemovePendingResponse(seq SequenceType) *PendingResponse { | ||
| return removePendingResponse(seq) | ||
| } |
Comment on lines
355
to
+357
| case <-gxtime.After(timeout): | ||
| err1 = errHeartbeatReadTimeout | ||
| remoting.RemovePendingResponse(remoting.SequenceType(req.ID)) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3440 +/- ##
===========================================
+ Coverage 46.76% 53.54% +6.78%
===========================================
Files 295 493 +198
Lines 17172 38395 +21223
===========================================
+ Hits 8031 20560 +12529
- Misses 8287 16182 +7895
- Partials 854 1653 +799 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The global
pendingResponsessync.Map is cleaned up on the success path viaResponse.Handle()→removePendingResponse(), but entries were never removed when requests fail. In network failure or timeout scenarios, this map grows indefinitely.Leak Points
ExchangeClient.Request()L134client.Request()returns errorExchangeClient.AsyncRequest()L168client.Request()returns errorgetty heartbeat()L356<-gxtime.After(timeout))Fix
remoting/exchange.go— ExportRemovePendingResponse()so external packages (e.g.,getty) can call it.remoting/exchange_client.go— AddremovePendingResponse()calls inRequest()andAsyncRequest()error paths before returning.remoting/getty/listener.go— Addremoting.RemovePendingResponse()call inheartbeat()timeout branch to clean up the orphaned entry.Testing
All
remoting/...package tests pass.🤖 Generated with Claude Code