Clean up dropped resources in relayer construction#1927
Clean up dropped resources in relayer construction#1927ogtownsend wants to merge 3 commits intomainfrom
Conversation
ogtownsend
had a problem deploying
to
integration
GitHub Actions
Failure
📊 API Diff Results
|
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent leaked/dropped resources during relayer construction in the LOOP relayer gRPC client/server wiring, supporting LINK-777.
Changes:
- Return accumulated
net.Resources(deps) from relayer/CCIP provider client-connection constructors soclientConn.refreshcan close them on failure. - Fix a server-side resource closer bug where the CSA keystore resource was closing the wrong connection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return 0, nil, fmt.Errorf("Failed to create relayer client: failed request: %w", err) | ||
| return 0, deps, fmt.Errorf("Failed to create relayer client: failed request: %w", err) | ||
| } | ||
| return reply.RelayerID, nil, nil | ||
| return reply.RelayerID, deps, nil |
There was a problem hiding this comment.
Returning deps here ensures resources are cleaned up when the request fails, but earlier error branches in this same NewClientConn callback still return nil deps after resources have already been started (e.g., failure serving the CSA keystore or capability registry). Because clientConn.refresh only closes the deps returned by this callback on error, those earlier paths can still leak the already-served resources. Return the accumulated deps (or explicitly close already-created resources) in those error branches as well, and consider passing prior resources as deps into subsequent ServeNew calls so they’re closed if a later ServeNew fails.
| }) | ||
| if err != nil { | ||
| return 0, nil, fmt.Errorf("Failed to create relayer client: failed to serve keystore: %w", err) | ||
| return 0, deps, fmt.Errorf("Failed to create relayer client: failed to serve keystore: %w", err) |
There was a problem hiding this comment.
@jmank88 do you have a preference for returning these deps or explicitly closing them here in the error block?
There was a problem hiding this comment.
We were closing everywhere before but it became very spammy. IIRC the original pattern was to just let them return naturally like:
| return 0, deps, fmt.Errorf("Failed to create relayer client: failed to serve keystore: %w", err) | |
| err = fmt.Errorf("Failed to create relayer client: failed to serve keystore: %w", err) | |
| return |
🤷
There was a problem hiding this comment.
gotcha, I guess I'll leave it as-is then 🤷
ogtownsend
had a problem deploying
to
integration
GitHub Actions
Failure
Requires
Supports