Revert "Clean up dropped resources in relayer construction (#1927)"#2026
Revert "Clean up dropped resources in relayer construction (#1927)"#2026ogtownsend wants to merge 1 commit intomainfrom
Conversation
This reverts commit c8e0d77.
|
👋 ogtownsend, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
There was a problem hiding this comment.
Pull request overview
This PR reverts commit c8e0d77d... (“Clean up dropped resources in relayer construction”) to keep develop aligned with the currently released CCIP version after a keystore socket drop regression in 2.41.3-rc1.
Changes:
- Reverts relayer/CCIP provider construction to stop returning/propagating certain
net.Resourcesdependency sets. - Adjusts several error paths in relayer construction to return
nildeps. - Alters CSA keystore resource wiring in the relayer server’s
NewRelayerpath.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil, net.ErrConnDial{Name: "CSAKeystore", ID: request.KeystoreCSAID, Err: err} | ||
| } | ||
| ksCSARes := net.Resource{Closer: ksCSAConn, Name: "CSAKeystore"} | ||
| ksCSARes := net.Resource{Closer: ksConn, Name: "CSAKeystore"} |
There was a problem hiding this comment.
ksCSARes is wrapping ksConn instead of ksCSAConn. This will cause the Keystore connection to be closed twice and will leak the CSA keystore connection (and any cleanup that relies on ksCSARes will act on the wrong connection). Use ksCSAConn as the Closer for the CSA resource.
| ksCSARes := net.Resource{Closer: ksConn, Name: "CSAKeystore"} | |
| ksCSARes := net.Resource{Closer: ksCSAConn, Name: "CSAKeystore"} |
| if err != nil { | ||
| return 0, deps, fmt.Errorf("Failed to create relayer client: failed to serve CSA keystore: %w", err) | ||
| return 0, nil, fmt.Errorf("Failed to create relayer client: failed to serve CSA keystore: %w", err) | ||
| } |
There was a problem hiding this comment.
On this error path, the keystore server (ksRes) may already have been started and added to deps, but the function returns nil deps. Because clientConn.refresh() only closes the deps that are returned, this can leak the already-started server(s) when a later ServeNew/RPC step fails. Return the accumulated deps on error, or explicitly close them before returning.
| if err != nil { | ||
| return 0, deps, fmt.Errorf("Failed to create relayer client: failed request: %w", err) | ||
| return 0, nil, fmt.Errorf("Failed to create relayer client: failed request: %w", err) | ||
| } | ||
| return reply.RelayerID, deps, nil | ||
| return reply.RelayerID, nil, nil | ||
| }) |
There was a problem hiding this comment.
On success this returns nil deps even though several resources were added to deps. This means the keystore/CSA keystore/capability registry servers created via ServeNew are never tracked for cleanup by the clientConn lifecycle (refresh/teardown), which can leave long-lived servers running and accumulate resources if NewRelayer is called multiple times. Either return deps here, or move ownership/cleanup of these resources somewhere that is guaranteed to close them when the relayer is closed.
| }) | ||
| if err != nil { | ||
| return 0, deps, err | ||
| return 0, nil, err |
There was a problem hiding this comment.
If ExtraDataCodecBundle is successfully served and then NewCCIPProvider returns an error, this currently returns nil deps, so edcRes won’t be cleaned up by the clientConn lifecycle and the server can be left running. Return the accumulated deps on error (or close edcRes before returning).
| return 0, nil, err | |
| return 0, deps, err |
This reverts commit c8e0d77.
This commit was reverted in
2.41.3-rc1due to the keystore dropped socket connectionWe need to make sure core
developbranch stays in sync with the currently released CCIP version