Skip to content

Revert "Clean up dropped resources in relayer construction (#1927)" #2026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ogtownsend wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −8
Draft

Revert "Clean up dropped resources in relayer construction (#1927)" #2026

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions pkg/loop/internal/relayer/relayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func (p *PluginRelayerClient) NewRelayer(ctx context.Context, config string, key
pb.RegisterKeystoreServer(s, ks.NewServer(keystore))
})
if err != nil {
return 0, deps, fmt.Errorf("Failed to create relayer client: failed to serve keystore: %w", err)
return 0, nil, fmt.Errorf("Failed to create relayer client: failed to serve keystore: %w", err)
}
deps.Add(ksRes)

Expand All @@ -70,15 +70,15 @@ func (p *PluginRelayerClient) NewRelayer(ctx context.Context, config string, key
pb.RegisterKeystoreServer(s, ks.NewServer(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)
}
Comment on lines 72 to 74
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
deps.Add(ksCSARes)

capabilityRegistryID, capabilityRegistryResource, err := p.ServeNew("CapabilitiesRegistry", func(s *grpc.Server) {
pb.RegisterCapabilitiesRegistryServer(s, capability.NewCapabilitiesRegistryServer(p.BrokerExt, capabilityRegistry))
})
if err != nil {
return 0, deps, fmt.Errorf("failed to serve new capability registry: %w", err)
return 0, nil, fmt.Errorf("failed to serve new capability registry: %w", err)
}
deps.Add(capabilityRegistryResource)

Expand All @@ -89,9 +89,9 @@ func (p *PluginRelayerClient) NewRelayer(ctx context.Context, config string, key
CapabilityRegistryID: capabilityRegistryID,
})
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
})
Comment on lines 91 to 95
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return newRelayerClient(p.BrokerExt, cc), nil
}
Expand Down Expand Up @@ -127,7 +127,7 @@ func (p *pluginRelayerServer) NewRelayer(ctx context.Context, request *pb.NewRel
p.CloseAll(ksRes)
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"}
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
ksCSARes := net.Resource{Closer: ksConn, Name: "CSAKeystore"}
ksCSARes := net.Resource{Closer: ksCSAConn, Name: "CSAKeystore"}

Copilot uses AI. Check for mistakes.

capRegistryConn, err := p.Dial(request.CapabilityRegistryID)
if err != nil {
Expand Down Expand Up @@ -324,7 +324,7 @@ func (r *relayerClient) NewCCIPProvider(ctx context.Context, cargs types.CCIPPro
ccipocr3pb.RegisterExtraDataCodecBundleServer(s, ccipocr3loop.NewExtraDataCodecBundleServer(cargs.ExtraDataCodecBundle))
})
if err != nil {
return 0, deps, fmt.Errorf("failed to serve ExtraDataCodecBundle: %w", err)
return 0, nil, fmt.Errorf("failed to serve ExtraDataCodecBundle: %w", err)
}
deps.Add(edcRes)
extraDataCodecBundleID = edcID
Expand All @@ -344,7 +344,7 @@ func (r *relayerClient) NewCCIPProvider(ctx context.Context, cargs types.CCIPPro
},
})
if err != nil {
return 0, deps, err
return 0, nil, err
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
return 0, nil, err
return 0, deps, err

Copilot uses AI. Check for mistakes.
}
return reply.CcipProviderID, deps, nil
})
Expand Down
Loading