✨ Adding begining of socket communication #860
Conversation
WalkthroughThis pull request introduces Unix domain socket (and Windows named pipe) support for serving providers, migrating from the internal jsonrpc2 package to jsonrpc2_v2 with async RPC patterns, removing handler-based logging infrastructure, updating all external providers to support socket-based serving, and upgrading dependencies across OpenTelemetry, gRPC, and related packages. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as External Provider
participant Server as provider.Server
participant Socket as socket Package
participant GRPC as gRPC Client
participant Binary as Language Server Binary
Provider->>Server: NewServer(..., socketPath, log)
activate Server
rect rgb(200, 220, 255)
Note over Server: Start() - Socket Mode (UseSocket=true)
Server->>Socket: Listen(socketPath)
activate Socket
Socket-->>Server: net.Listener (Unix socket)
deactivate Socket
end
rect rgb(200, 220, 255)
Note over Server: Start() - Binary Startup with Socket
Server->>Socket: GetAddress(pipeName)
activate Socket
Socket-->>Server: socket address
deactivate Socket
Server->>Binary: Start binary with --socket and --name
activate Binary
Binary->>Socket: GetConnectionString(address)
activate Socket
Socket-->>Binary: "unix://socket_path"
deactivate Socket
Binary-->>Server: Process started
deactivate Binary
end
rect rgb(200, 220, 255)
Note over Server: Connect to Binary via Socket
Server->>Socket: ConnectGRPC(connectionString)
activate Socket
Socket->>GRPC: grpc.NewClient with custom dialer
activate GRPC
GRPC-->>Socket: *grpc.ClientConn
deactivate GRPC
Socket-->>Server: gRPC connection
deactivate Socket
end
deactivate Server
sequenceDiagram
participant Java as Java External Provider
participant Dialer as base.StdDialer
participant JSONRPC as jsonrpc2_v2
participant LSP as LSP Server
Java->>Dialer: base.NewStdDialer(stdin, stdout)
activate Dialer
Dialer-->>Java: *StdDialer (implements jsonrpc2.Dialer)
deactivate Dialer
Java->>JSONRPC: jsonrpc2.Dial(ctx, dialer, handler)
activate JSONRPC
rect rgb(200, 220, 255)
Note over JSONRPC: Async RPC Pattern
JSONRPC->>LSP: Send initialize request
activate LSP
LSP-->>JSONRPC: initialize response (async)
deactivate LSP
JSONRPC-->>Java: *jsonrpc2.Connection
deactivate JSONRPC
end
Java->>Java: rpc.Call("workspace/executeCommand").Await(ctx, &refs)
activate Java
Note over Java: Async call with Await
Java-->>Java: Result populated
deactivate Java
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This change spans diverse modifications across multiple areas: new socket abstraction layer with platform-specific implementations (Windows pipes, Unix sockets), migration from deprecated internal jsonrpc2 to jsonrpc2_v2 with async patterns, removal of handler-based logging infrastructure, updates to RPC client interfaces and patterns across multiple providers, dependency version bumps, and build configuration changes. While many changes follow repetitive patterns (e.g., adding socket parameter to multiple main.go files), the heterogeneity of concerns (networking abstractions, RPC migration, build updates, removals) and density of API signature changes require careful review of interactions and compatibility.
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
from
49dc9a8 to
ded14e8
Compare
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
2 times, most recently
from
5fa67ec to
be9138d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
provider/grpc/socket/pipe_windows.go (1)
49-49: Remove debug print statement.The debug print statement should be removed from production code as it may leak sensitive information and affects performance.
- fmt.Printf("pipe: %#v", pipe)If debugging is needed, use the structured logger passed through context or make it conditional on a debug flag.
provider/grpc/provider.go (1)
290-292: Consider returning error instead of fatal exit.Using
log.Fatalffor provider startup failures may be too aggressive, as it terminates the entire process. Consider returning the error to allow the caller to handle it gracefully.err = cmd.Start() if err != nil { - log.Fatalf("unable to start provider: %v", err) + return nil, nil, fmt.Errorf("unable to start provider: %w", err) }This allows for better error handling and recovery strategies in the calling code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/golang-dependency-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
Dockerfile(1 hunks)Makefile(1 hunks)external-providers/dotnet-external-provider/Dockerfile(1 hunks)external-providers/dotnet-external-provider/go.mod(1 hunks)external-providers/dotnet-external-provider/main.go(3 hunks)external-providers/generic-external-provider/Dockerfile(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/generic-external-provider/main.go(3 hunks)external-providers/golang-dependency-provider/Dockerfile(1 hunks)external-providers/golang-dependency-provider/go.mod(1 hunks)external-providers/java-external-provider/Dockerfile(1 hunks)external-providers/java-external-provider/go.mod(1 hunks)external-providers/java-external-provider/main.go(3 hunks)external-providers/yq-external-provider/Dockerfile(1 hunks)external-providers/yq-external-provider/go.mod(3 hunks)external-providers/yq-external-provider/main.go(3 hunks)go.mod(3 hunks)provider/grpc/provider.go(7 hunks)provider/grpc/service_client.go(1 hunks)provider/grpc/socket/client_connect.go(1 hunks)provider/grpc/socket/pipe_windows.go(1 hunks)provider/grpc/socket/uds.go(1 hunks)provider/provider.go(1 hunks)provider/server.go(8 hunks)provider_container_settings.json(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
external-providers/generic-external-provider/Dockerfile (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/yq-external-provider/Dockerfile (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/golang-dependency-provider/Dockerfile (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/dotnet-external-provider/go.mod (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Dockerfile (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Makefile (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/dotnet-external-provider/Dockerfile (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/java-external-provider/go.mod (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
external-providers/golang-dependency-provider/go.mod (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/yq-external-provider/go.mod (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
external-providers/generic-external-provider/go.mod (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
go.mod (2)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
🧬 Code Graph Analysis (7)
provider/grpc/socket/client_connect.go (1)
provider/grpc/socket/pipe_windows.go (2)
ConnectGRPC(28-37)MAX_MESSAGE_SIZE(19-19)
external-providers/dotnet-external-provider/main.go (1)
provider/server.go (1)
NewServer(64-99)
external-providers/yq-external-provider/main.go (1)
provider/server.go (1)
NewServer(64-99)
external-providers/java-external-provider/main.go (1)
provider/server.go (1)
NewServer(64-99)
provider/grpc/socket/pipe_windows.go (2)
provider/grpc/socket/uds.go (2)
GetSocketAddress(11-20)Listen(22-25)provider/grpc/socket/client_connect.go (2)
ConnectGRPC(14-19)MAX_MESSAGE_SIZE(11-11)
provider/grpc/socket/uds.go (2)
provider/grpc/socket/pipe_windows.go (2)
GetSocketAddress(22-27)Listen(39-41)jsonrpc2_v2/serve.go (1)
Listener(18-31)
provider/grpc/provider.go (3)
provider/grpc/socket/uds.go (1)
GetSocketAddress(11-20)provider/grpc/socket/pipe_windows.go (3)
GetSocketAddress(22-27)ConnectGRPC(28-37)MAX_MESSAGE_SIZE(19-19)provider/grpc/socket/client_connect.go (2)
ConnectGRPC(14-19)MAX_MESSAGE_SIZE(11-11)
🔇 Additional comments (48)
external-providers/yq-external-provider/Dockerfile (1)
1-1: Resolved:golang:1.23.11base image exists on Docker HubVerified via Docker Hub API that the
golang:1.23.11tag is published. No changes required.external-providers/dotnet-external-provider/Dockerfile (1)
2-2: Same availability concern forgolang:1.23.11image.Ensure the tag is published for all target architectures, otherwise the multi-stage build will break.
external-providers/golang-dependency-provider/Dockerfile (1)
1-1: Confirm Go 1.23.11 image exists or fall back to the latest 1.22/1.23 tag.Build reproducibility depends on the tag being present and immutable.
external-providers/generic-external-provider/Dockerfile (1)
1-1: Potential build breakage ifgolang:1.23.11is unpublished.All provider images moved to this tag; failure in one stage will cascade.
Consider keeping1.23or wait until1.23.11is officially released.Dockerfile (1)
4-4: LGTM – Go 1.23.11 Docker image verifiedThe
golang:1.23.11tag exists on Docker Hub and aligns with our broader modernization effort. No further changes required.external-providers/java-external-provider/Dockerfile (1)
5-5: LGTM! Consistent Go version upgrade.The Go base image version update is consistent with the main Dockerfile and other provider Dockerfiles, ensuring uniform build environments across all components.
provider/provider.go (1)
90-90: LGTM! Clean addition enabling socket communication.The new
UseSocketboolean field is well-integrated into the existingConfigstruct with appropriate YAML and JSON tags. This provides a clean configuration mechanism to choose between TCP ports and Unix domain sockets/named pipes for provider communication.Makefile (1)
21-21: Approve consistent Go version updates; patch‐version flag is validI ran
go mod tidy -go=1.23.11under Go 1.24.1 and confirmed:
- The
godirective is updated togo 1.23.11- A
toolchain go1.24.1line is added (expected for the running Go version)This verifies that using
-go=1.23.11is supported and produces the intended go.mod. No further changes needed.provider_container_settings.json (1)
5-5: LGTM – selective socket enablement for Go and Java providersI’ve verified that both the Go provider (generic-external-provider) and the Java provider include the necessary socket flags, and your JSON changes correctly enable
useSocket: truefor those two.Notably, socket support is also implemented in the .NET and yq providers (their
main.gofiles contain the same socket flags), but they aren’t enabled in the config. Is this selective rollout intentional, or should we also add"useSocket": truefor:
- dotnet
- yq
Please confirm the rollout plan or update
provider_container_settings.jsonaccordingly.external-providers/generic-external-provider/main.go (3)
17-17: LGTM: Socket flag addition is correct.The socket flag follows the established pattern and has appropriate defaults.
56-59: LGTM: Validation logic correctly ensures serving location is specified.The validation properly requires either a socket or port (or both) to be provided. The error handling is consistent with the existing codebase style.
77-77: LGTM: Server initialization correctly includes socket parameter.The socket parameter is properly passed to
provider.NewServerwith correct parameter ordering.provider/grpc/service_client.go (1)
36-42: LGTM: Excellent defensive programming for nil response handling.This change prevents potential nil pointer dereferences when the gRPC response is successful but the Response field is nil. The logic correctly returns a "no match" result and includes a clear explanatory comment.
provider/grpc/socket/client_connect.go (3)
1-2: LGTM: Build constraint correctly targets non-Windows platforms.The build constraint appropriately excludes Windows platforms, complementing the Windows-specific implementation in
pipe_windows.go.
3-8: LGTM: Package declaration and imports are appropriate.Clean imports with only the necessary gRPC dependencies for socket-based connections.
10-19: LGTM: Socket client connection implementation is clean and consistent.The implementation correctly:
- Uses the same 8MB message size limit as the Windows implementation
- Appropriately uses insecure credentials for local socket communication
- Provides a simple, focused connection interface
The Unix implementation is simpler than the Windows version (which uses passthrough resolver and custom dialer), which is appropriate given the different underlying socket mechanisms.
external-providers/dotnet-external-provider/main.go (3)
21-21: LGTM: Socket flag addition follows established pattern.Consistent with other external providers in the codebase.
38-41: LGTM: Validation logic is consistent across external providers.The validation ensures proper server configuration and maintains consistency with other external providers in the codebase.
59-59: LGTM: Server initialization correctly includes socket parameter.The socket parameter integration is consistent with other external providers and matches the expected function signature.
external-providers/yq-external-provider/main.go (3)
17-17: LGTM: Socket flag addition is consistent with other providers.The socket flag follows the established pattern with appropriate defaults.
36-39: LGTM: Validation logic maintains consistency across external providers.The validation ensures proper server configuration with consistent error handling patterns.
56-56: LGTM: Server initialization correctly integrates socket parameter.The socket parameter is properly passed to
provider.NewServerwith consistent implementation across external providers.external-providers/java-external-provider/go.mod (3)
3-5: LGTM! Go version and toolchain directives are correctly formatted.The use of patch version
go 1.23.11is valid since Go 1.21+, and the separatetoolchain go1.24.3directive correctly specifies the build toolchain version.
8-49: Systematic dependency upgrades look good.The dependency updates are consistent and align with the broader modernization effort across all external providers. Key upgrades include OpenTelemetry packages to v1.34.0 and gRPC to v1.72.2.
51-51: Local replace directive is appropriate for development.The replace directive pointing to
../../facilitates local development against the updated analyzer-lsp module during this PR development cycle.external-providers/generic-external-provider/go.mod (3)
3-3: Go version update is correctly formatted.Using
go 1.23.11is valid syntax since Go 1.21+ supports patch versions in the go directive.
25-36: Dependency upgrades are consistent with the modernization effort.The OpenTelemetry and gRPC version bumps align with other external providers, ensuring consistency across the project.
39-39: Local replace directive supports development workflow.The replace directive facilitates local development against the updated analyzer-lsp module.
external-providers/golang-dependency-provider/go.mod (3)
3-3: Go version update is consistent with other providers.The
go 1.23.11format is valid and maintains consistency across all external providers.
23-33: Dependency version bumps align with project-wide updates.The OpenTelemetry and gRPC upgrades match other providers, ensuring version consistency across the project.
37-37: Replace directive supports local development.The local path replacement is appropriate for this PR's development workflow.
external-providers/java-external-provider/main.go (3)
17-17: Socket flag implementation is correct.The socket flag declaration follows the established pattern and provides appropriate documentation.
41-43: Validation logic correctly implements either/or requirement.The condition properly ensures that either a socket path or port must be provided, making them mutually exclusive alternatives. The error message accurately reflects this requirement.
61-61: Server initialization correctly passes socket parameter.The call to
provider.NewServerproperly includes the socket parameter, matching the function signature shown in the relevant code snippets.external-providers/yq-external-provider/go.mod (3)
3-3: Go version update completes the systematic upgrade.The
go 1.23.11version is consistent with all other external providers, completing the project-wide Go version standardization.
16-24: Dependency upgrades maintain version consistency.The OpenTelemetry, gRPC, and other dependency updates align with the versions used across all external providers, ensuring compatibility and consistency.
Also applies to: 32-41
43-43: Replace directive completes the local development setup.The local path replacement is consistent with all other external providers and supports the development workflow for this PR.
go.mod (3)
21-25: Compatibility Verified: gRPC v1.72.2 & protobuf v1.36.5No breaking changes detected. Starting in gRPC v1.34.0, the module switched from
github.com/golang/protobuftogoogle.golang.org/protobuf, so using gRPC v1.72.2 with protobuf v1.36.5 is fully supported. No further action required.
3-3: No changes needed for Go version directiveThe
go 1.23.11directive is fully supported by Go tooling (patch‐level versions have been accepted since Go 1.21). The failure ingo mod verifyis due to the sandbox’s empty GOPROXY setting and does not indicate an issue with the go.mod format. You can disregard that error—no update to thegodirective is required.
6-6: No known security issues with github.com/Microsoft/go-winio v0.6.2
A review of the Go Vulnerability Database and GitHub advisories shows no reported vulnerabilities for version 0.6.2. This dependency addition is safe. Continue to monitor the Go Vulnerability Database (https://pkg.go.dev/vuln/list?utm_source=openai) and the official repo for any future security updates.provider/server.go (4)
64-64: LGTM - Server constructor properly extended for socket support.The
NewServerfunction correctly accepts the newsocketPathparameter and integrates it into the server struct. The parameter ordering and validation logic are appropriate.
104-110: LGTM - Robust server startup validation.The conditional logic properly ensures that either a port or socket path is provided, preventing invalid server configurations. The error message is clear and actionable.
131-131: LGTM - Consistent use of socket package constants.The migration from hardcoded
provider.MAX_MESSAGE_SIZEtosocket.MAX_MESSAGE_SIZEensures consistency across the socket communication implementation.
492-492: LGTM - JWT callback signature updated correctly.The change from
(interface{}, error)to(any, error)aligns with modern Go conventions and the jwt library's expected signature. This is a good modernization.provider/grpc/provider.go (4)
49-51: LGTM - Improved provider output logging.The refactoring of
LogProviderOutto a standalone function and running it as a goroutine immediately after process start is a good improvement. This ensures provider output is captured even if there are issues during connection establishment.
267-281: LGTM - Clean socket vs TCP connection logic.The conditional logic properly handles both socket and TCP connections based on the
UseSocketconfiguration flag. The connection string generation is appropriate for each transport type.
294-294: LGTM - Unified gRPC connection handling.The use of
socket.ConnectGRPCprovides a consistent interface for both socket and TCP connections, which is a good architectural decision.
304-305: LGTM - Consistent message size configuration.The migration to
socket.MAX_MESSAGE_SIZEensures consistent maximum message size across all connection types (socket and TCP).
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
from
7cc9338 to
050d4c0
Compare
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
from
ceedb6c to
c0ca331
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
provider/grpc/socket/pipe_windows.go (1)
43-51: Remove debug print statement before production release.Line 49 contains a debug
fmt.Printfthat should be removed or replaced with proper logging. Additionally, line 44'sstrings.CutPrefixresult is ignored; consider logging if the prefix is missing.Apply this diff:
func DialWindowsPipePassthrough(ctx context.Context, connectionString string) (net.Conn, error) { - pipeName, _ := strings.CutPrefix(connectionString, "unix://") + pipeName, found := strings.CutPrefix(connectionString, "unix://") + if !found { + return nil, fmt.Errorf("invalid pipe connection string: expected unix:// prefix") + } pipe, err := winio.DialPipeContext(ctx, pipeName) if err != nil { return nil, err } - fmt.Printf("pipe: %#v", pipe) return pipe, nil }provider/grpc/provider.go (1)
285-289: Remove debug print statement before production release.Line 285 contains a debug
fmt.Printfthat outputs the full command. This should be removed or replaced with proper logging at an appropriate level.Apply this diff:
- fmt.Printf("\ncommand: %v\n", cmd) + log.V(5).Info("starting provider command", "command", cmd.String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
external-providers/java-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
external-providers/dotnet-external-provider/main.go(3 hunks)external-providers/generic-external-provider/main.go(3 hunks)external-providers/java-external-provider/go.mod(2 hunks)external-providers/java-external-provider/main.go(3 hunks)external-providers/yq-external-provider/main.go(3 hunks)go.mod(4 hunks)provider/grpc/provider.go(4 hunks)provider/grpc/service_client.go(1 hunks)provider/grpc/socket/consts.go(1 hunks)provider/grpc/socket/pipe_windows.go(1 hunks)provider/grpc/socket/uds.go(1 hunks)provider/provider.go(1 hunks)provider/server.go(8 hunks)provider_container_settings.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- provider/provider.go
- external-providers/dotnet-external-provider/main.go
- provider/grpc/socket/consts.go
- external-providers/generic-external-provider/main.go
- provider_container_settings.json
- provider/grpc/socket/uds.go
- external-providers/java-external-provider/main.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-28T15:28:31.377Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:22-26
Timestamp: 2025-07-28T15:28:31.377Z
Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-07-28T15:27:30.233Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:28-37
Timestamp: 2025-07-28T15:27:30.233Z
Learning: In Windows named pipe gRPC connections, the "unix://" URI scheme prefix must be used instead of a more semantically correct scheme like "pipe://" because it's required for the gRPC package to provide the desired behavior. This is an intentional design choice despite being semantically incorrect.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-07-25T02:05:02.884Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The `go` directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate `toolchain` directive (introduced in Go 1.21), e.g., `toolchain go1.23.11`.
Applied to files:
external-providers/java-external-provider/go.mod
🧬 Code graph analysis (5)
provider/grpc/provider.go (4)
provider/provider.go (1)
Config(86-96)provider/grpc/socket/pipe_windows.go (3)
GetAddress(17-22)GetConnectionString(24-26)ConnectGRPC(28-37)provider/grpc/socket/uds.go (3)
GetAddress(14-23)GetConnectionString(25-27)ConnectGRPC(33-40)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)
provider/grpc/service_client.go (3)
jsonrpc2_v2/messages.go (1)
Response(42-49)provider/provider.go (1)
ProviderEvaluateResponse(295-299)provider/internal/grpc/library.pb.go (3)
ProviderEvaluateResponse(470-477)ProviderEvaluateResponse(490-490)ProviderEvaluateResponse(505-507)
provider/server.go (4)
provider/provider.go (1)
BaseClient(442-446)provider/grpc/socket/pipe_windows.go (1)
Listen(39-41)provider/grpc/socket/uds.go (1)
Listen(29-31)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)
provider/grpc/socket/pipe_windows.go (2)
provider/grpc/socket/uds.go (4)
GetAddress(14-23)GetConnectionString(25-27)ConnectGRPC(33-40)Listen(29-31)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)
external-providers/yq-external-provider/main.go (1)
provider/server.go (1)
NewServer(64-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test (windows-latest)
🔇 Additional comments (15)
go.mod (1)
6-6: LGTM! Dependency updates align with socket communication feature.The addition of
go-winio v0.6.2enables Windows named pipe support, and the updates to gRPC (v1.73.0), OpenTelemetry (v1.35.0), and related dependencies are appropriate for the new socket-based transport functionality.Based on learnings: gRPC v1.73.0 enables least-request LB by default and adds CallAuthority option; go-winio v0.6.2 provides Windows pipe utilities with Go 1.21+ support.
Also applies to: 21-25, 38-39, 50-55
external-providers/java-external-provider/go.mod (1)
5-5: LGTM! Toolchain and dependency updates are appropriate.The
toolchain go1.24.3directive correctly specifies the required Go toolchain version while keeping the module version at 1.23.9. All dependency updates are minor/patch version bumps that align with the broader socket communication feature.Based on learnings: The toolchain directive is the correct approach for specifying patch-level Go version requirements (introduced in Go 1.21).
Also applies to: 8-8, 10-10, 18-18, 24-24, 34-34, 38-41
provider/grpc/service_client.go (1)
36-41: LGTM! Defensive nil check prevents potential panic.This correctly handles the case where a provider returns success without a response payload, treating it as no match. This prevents a nil pointer dereference at line 43 where
r.Response.Matchedwould be accessed.external-providers/yq-external-provider/main.go (1)
17-17: LGTM! Socket support correctly integrated.The socket flag, validation logic, and updated
NewServercall correctly implement socket-based startup alongside the existing port-based approach. The validation ensures exactly one serving location is provided.Also applies to: 36-39, 56-56
provider/server.go (4)
64-64: LGTM! Signature update correctly adds socket support.The
NewServersignature now accepts asocketPathparameter, enabling socket-based serving alongside TCP. This is consistently propagated to the server struct.Also applies to: 91-91
102-110: LGTM! Clean listener selection logic.The mutual exclusion between Port and SocketPath is correctly validated, with appropriate error handling. The
socket.Listenabstraction cleanly handles platform-specific differences (Unix domain sockets vs. Windows named pipes).
131-131: LGTM! Correct use of shared MAX_MESSAGE_SIZE constant.Using
socket.MAX_MESSAGE_SIZEfrom the shared constants file eliminates duplication and ensures consistency across the codebase.
492-492: LGTM! Cosmetic type alias update.Changing
interface{}toanyis a cosmetic improvement using Go 1.18+'s type alias. Both are equivalent.provider/grpc/socket/pipe_windows.go (4)
17-21: LGTM! Pipe name generation is appropriate.The use of
math/randfor generating random pipe suffixes is sufficient for collision avoidance in local IPC scenarios.Based on learnings: Cryptographic security is not required for Windows named pipe address generation in this project.
24-26: LGTM! Required gRPC URI scheme format.The
passthrough:unix://prefix is required for gRPC to provide correct behavior with the custom dialer, despite being semantically incorrect for Windows pipes.Based on learnings: The unix:// scheme must be used for gRPC compatibility with the custom dialer, even on Windows.
28-37: LGTM! Windows pipe gRPC client properly configured.The client correctly uses a custom dialer with passthrough resolver, appropriate message size limits, insecure credentials (for local IPC), and localhost authority.
39-41: LGTM! Named pipe listener creation.Using
winio.ListenPipewith default config is appropriate for creating a Windows named pipe listener.provider/grpc/provider.go (3)
44-44: LGTM! Logger threading properly implemented.The
startfunction signature now accepts a logger, enabling proper logging throughout the startup process and inLogProviderOut. This is a clean improvement over the previous implementation.Also applies to: 249-249, 344-344
263-277: LGTM! Clean socket vs. port selection logic.The conditional logic clearly separates socket-based and port-based startup paths, with appropriate error handling and command construction for each case.
295-300: LGTM! Connection logic properly updated.All connection paths now correctly use:
socket.ConnectGRPCfor socket-based connections with proper abstractionsgrpc.NewClient(newer API) for direct address connectionssocket.MAX_MESSAGE_SIZEfor consistent message size limits- Proper error handling and logging throughout
The refactoring consolidates connection logic while maintaining support for TLS and JWT authentication paths.
Also applies to: 304-312, 314-340
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
demo.Dockerfile (1)
20-20: Non-POSIX redirection may fail under /bin/sh&> is bash-specific. Use portable redirection.
-ENTRYPOINT ["sh", "-c", "all-in-one-linux &> /dev/null & sleep 5 && konveyor-analyzer --enable-jaeger && curl -o traces.json http://localhost:16686/api/traces?service=analyzer-lsp"] +ENTRYPOINT ["sh", "-c", "all-in-one-linux >/dev/null 2>&1 & sleep 5 && konveyor-analyzer --enable-jaeger && curl -o traces.json http://localhost:16686/api/traces?service=analyzer-lsp"]external-providers/java-external-provider/pkg/java_external_provider/service_client.go (3)
205-211: Guard type assertion when filtering by filepathsref.Location.Value may not be protocol.Location; current code can panic.
- if strings.HasSuffix(ref.Location.Value.(protocol.Location).URI, fp) { + if loc, ok := ref.Location.Value.(protocol.Location); ok && strings.HasSuffix(loc.URI, fp) { filteredRefs = append(filteredRefs, ref) }
192-200: jsonrpc2.IsRPCClosed may not exist in v2; make detection robustThe v2 package likely doesn’t expose IsRPCClosed. Use a local helper to detect closed/EOF conditions and avoid compile errors.
- if jsonrpc2.IsRPCClosed(err) { + if isRPCClosed(err) { log.Error(err, "connection to the language server is closed, language server is not running") return refs, fmt.Errorf("connection to the language server is closed, language server is not running") } else {And similarly in GetAllReferences and initialization.
Add this helper in the same file:
// near the top of the file func isRPCClosed(err error) bool { if err == nil { return false } return errors.Is(err, io.EOF) || errors.Is(err, context.Canceled) || errors.Is(err, syscall.EPIPE) || strings.Contains(strings.ToLower(err.Error()), "use of closed") || strings.Contains(strings.ToLower(err.Error()), "broken pipe") || strings.Contains(strings.ToLower(err.Error()), "file already closed") }Also applies to: 255-261, 413-417
311-321: Ensure graceful shutdown closes RPC and cancels contextAfter shutdown/exit, close the client and cancel to stop goroutines and close pipes deterministically.
err = p.rpc.Notify(shutdownCtx, "exit", nil) if err != nil { p.log.Error(err, "failed to send exit notification to language server") return err } +// Close RPC connection and cancel the service context +if cerr := p.rpc.Close(); cerr != nil { + p.log.V(7).Error(cerr, "rpc close error (ignored)") +} +if p.cancelFunc != nil { + p.cancelFunc() +} return nilexternal-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
936-944: Indexing bug parsing Gradle unresolved sourcesAccessing match[4] is out of bounds; use gav[4] from artifactRegex.
- if gav[4] != "" { // internal library, unknown group/version - artifact := javaArtifact{ - ArtifactId: match[4], - } + if gav[4] != "" { // internal library, unknown group/version + artifact := javaArtifact{ + ArtifactId: gav[4], + }Add/adjust a unit test covering this branch to prevent regressions.
🧹 Nitpick comments (11)
lsp/base_service_client/base_handlers.go (5)
83-86: Prefer integer retries and bit-shift over float Pow.Using float64 for retries + math.Pow is unnecessary and risks rounding. An int with 1<<retries is simpler and faster.
Apply this refactor:
-type backoffTimer struct { - mu sync.Mutex - retries float64 - lastAttemptedTime *time.Time - lastDurationTime time.Duration -} +type backoffTimer struct { + mu sync.Mutex + retries int + lastAttemptedTime *time.Time + lastDurationTime time.Duration +} @@ - // calculated back off - b.lastDurationTime = time.Second * time.Duration((math.Pow(2, b.retries))) + // calculated backoff (2^retries seconds) + b.lastDurationTime = time.Second * time.Duration(1<<uint(b.retries))Also applies to: 106-116
77-80: Keying by raw Params string is brittle and memory-heavy.string(req.Params) duplicates payload bytes; semantically equivalent JSON can hash differently (whitespace/order). Consider a stable, compact key.
Option A (hash only):
sum := sha256.Sum256(req.Params) key := requestKey{method: req.Method, params: hex.EncodeToString(sum[:])}Option B (canonicalize then hash):
- Decode to interface{}, re-marshal with stable ordering, then hash.
- Or rely on a canonical JSON lib if available.
Also applies to: 129-133
64-66: Unbounded map growth; add TTL/eviction.failedRequests grows per unique method+params with no cleanup; long-running processes can leak memory.
- Track last access and evict entries idle > N minutes via a background sweeper or lazy cleanup on insert.
- Alternatively, cap size with LRU semantics.
118-126: Stale/misleading comment block.This “Log is invoked...” comment doesn’t describe BackoffHandler.Handle; it seems copied from a different API.
Apply this diff:
-// Log is invoked for all messages flowing through a Conn. -// direction indicates if the message being received or sent -// id is the message id, if not set it was a notification -// elapsed is the time between a call being seen and the response, and is -// negative for anything that is not a response. -// method is the method name specified in the message -// payload is the parameters for a call or notification, and the result for a -// response -// Request is called near the start of processing any request. +// Handle applies per-request exponential backoff before passing the request +// down the handler chain. It always returns jsonrpc2.ErrNotHandled.
99-104: Small nits: simplify duration literal; avoid redundant mutex init.
- time.Minute * 1 -> time.Minute
- failedRequestsMu is zero-value initialized; explicit init in ctor is unnecessary.
- if time.Now().After(b.lastAttemptedTime.Add(b.lastDurationTime).Add(time.Minute * 1)) { + if time.Now().After(b.lastAttemptedTime.Add(b.lastDurationTime).Add(time.Minute)) {- failedRequestsMu: sync.Mutex{},No behavior change.
Also applies to: 110-112, 72-72
demo.Dockerfile (2)
2-2: Local base image will break remote buildsFROM localhost/... won’t be pullable in CI or by contributors. Use a public registry image or make it a build-arg defaulting to quay.io.
- FROM localhost/testing/analyzer-lsp:testing-socket + ARG BASE_IMAGE=quay.io/konveyor/analyzer-lsp:latest + FROM ${BASE_IMAGE}
15-15: Slim image and cache hygieneClean dnf metadata and layer installs together to reduce size.
-RUN microdnf install go-toolset vim procps -y +RUN microdnf -y install go-toolset vim procps \ + && microdnf -y clean allprovider/provider.go (2)
154-156: Consider omitempty for PipeNameAvoid emitting empty fields in JSON/YAML.
- PipeName string `yaml:"pipeName" json:"pipeName"` + PipeName string `yaml:"pipeName,omitempty" json:"pipeName,omitempty"`
91-91: Document UseSocketAdd a short comment clarifying precedence: Address vs UseSocket/PipeName.
- UseSocket bool `yaml:"useSocket,omitempty" json:"useSocket,omitempty"` + // When true, start/connect via Unix domain socket (or Windows named pipe via PipeName) instead of TCP Address. + UseSocket bool `yaml:"useSocket,omitempty" json:"useSocket,omitempty"`provider_container_settings.json (1)
12-12: Absolute gopls pathHardcoding /root/go/bin/gopls ties to root user. If possible, derive from GOBIN/GOPATH or make it configurable.
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
410-421: Initialization retry without backoffTight loop retries can spin CPU. Add a short sleep or exponential backoff.
for i := 0; i < 10; i++ { err := p.rpc.Call(ctx, "initialize", params).Await(ctx, &result) if err != nil { ... - continue + time.Sleep(500 * time.Millisecond) + continue } break }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
Dockerfile(0 hunks)Dockerfile.windows(0 hunks)debug/debug.Dockerfile(0 hunks)demo.Dockerfile(2 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/java-external-provider/go.mod(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(6 hunks)external-providers/yq-external-provider/go.mod(2 hunks)external-providers/yq-external-provider/pkg/yq_provider/service_client.go(0 hunks)jsonrpc2/backoff_handler.go(0 hunks)jsonrpc2/handler.go(0 hunks)jsonrpc2/jsonrpc2.go(0 hunks)jsonrpc2/rpcerr.go(0 hunks)jsonrpc2/stream.go(0 hunks)jsonrpc2/wire.go(0 hunks)lsp/base_service_client/base_handlers.go(2 hunks)lsp/base_service_client/std_dialer.go(1 hunks)provider/provider.go(3 hunks)provider_container_settings.json(3 hunks)
💤 Files with no reviewable changes (10)
- debug/debug.Dockerfile
- jsonrpc2/stream.go
- Dockerfile.windows
- Dockerfile
- jsonrpc2/rpcerr.go
- jsonrpc2/wire.go
- jsonrpc2/handler.go
- external-providers/yq-external-provider/pkg/yq_provider/service_client.go
- jsonrpc2/backoff_handler.go
- jsonrpc2/jsonrpc2.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go
🧬 Code graph analysis (4)
lsp/base_service_client/base_handlers.go (2)
jsonrpc2_v2/jsonrpc2.go (2)
Handler(60-75)ErrNotHandled(24-24)jsonrpc2_v2/messages.go (1)
Request(30-38)
lsp/base_service_client/std_dialer.go (1)
jsonrpc2_v2/serve.go (1)
Dial(57-64)
provider/provider.go (1)
jsonrpc2_v2/conn.go (1)
AsyncCall(375-381)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (5)
lsp/base_service_client/std_dialer.go (1)
NewStdDialer(24-31)jsonrpc2_v2/serve.go (1)
Dial(57-64)jsonrpc2_v2/conn.go (1)
ConnectionOptions(46-60)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)lsp/base_service_client/base_handlers.go (2)
NewChainHandler(36-48)LogHandler(22-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (10)
external-providers/java-external-provider/go.mod (4)
3-5: Verify toolchain version is intentional.The Go module version is pinned to 1.23.9, but the toolchain directive specifies go1.24.3. While this is syntactically valid (it allows compilation with a newer toolchain while maintaining 1.23.9 compatibility), confirm that this version mismatch is intentional.
13-13: Clarify indirect marking on direct grpc require.The
google.golang.org/grpcdependency is listed in the direct require block with an// indirectcomment, which is unusual. Confirm whether this is intentional (e.g., for version pinning) or whether it should be moved to an indirect section.
24-24: Windows named pipes dependency aligns with socket communication feature.The addition of
github.com/Microsoft/go-winio v0.6.2as an indirect dependency is appropriate for the new Windows named pipes support in the socket communication feature introduced by this PR.
1-52: Verify all dependency updates are compatible and necessary.This commit includes routine dependency updates across OpenTelemetry, grpc, and other packages. Most are patch or minor version bumps. Ensure that:
- All updates are compatible with the new socket communication feature.
- There are no known security advisories for the updated versions.
- The dependency resolver has no conflicts or warnings.
Running
go mod tidyandgo mod verifyin this module would help confirm consistency.provider_container_settings.json (1)
5-5: No action required; socket implementation is correctThe
useSocket: trueflag already takes effect as intended. When enabled, the code reads the flag (provider/grpc/provider.go line 263), obtains the socket path viasocket.GetAddress(), and launches the provider binary with the--socketargument (line 268). The runtime wiring is properly configured.external-providers/yq-external-provider/go.mod (1)
16-16: Dependencies are tidy and cross-platform verifiedgo mod tidy confirmed no changes to go.mod/go.sum—the files are already properly tidied. go-winio v0.6.2 is correctly marked as indirect. All transitive dependencies resolved without issue.
provider/provider.go (1)
159-162: Interface change ripple-check: All implementers and call sites properly updatedVerification confirms all changed files correctly implement the new
RPCClientinterface:
java-external-provider/service_client.go: All fourCall()sites properly chain.Await()(lines 191, 254, 312, 411)yq-external-provider: Migrated away from old package entirelybase_service_client: Updated imports tojsonrpc2_v2IsRPCClosed()function and usage remain validThe interface transition from
Call(...interface{}, interface{}) errortoCall(...any) *AsyncCallhas been fully and correctly implemented across all modified files in this PR.external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go (1)
541-541: Review comment is incorrect—depInit() method does not existThe
javaServiceClienttype has nodepInit()method. The commented line on 541 (//p.depInit()) is already documented with the explanation "we are not testing dep init here, so ignore error" and poses no risk of silently changing defaults or caches. The code is correct as written.Likely an incorrect or invalid review comment.
lsp/base_service_client/std_dialer.go (1)
47-54: LGTM!The
Closemethod correctly closes both underlying streams and properly combines errors usingerrors.Join.external-providers/generic-external-provider/go.mod (1)
16-37: Dependency versions are secure but OpenTelemetry could be more current.Security check confirmed no known vulnerabilities for
go-winio v0.6.2or the OpenTelemetry packages. However, OpenTelemetry latest versions were published Aug 29, 2025, while the go.mod specifies v1.35.0. More recent semantic convention versions (v1.37.0) are available. Consider updating to the latest OpenTelemetry versions for improved stability and features.
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
external-providers/generic-external-provider/entrypoint.sh (1)
3-4: Hardcoded socket path and missing error handling pose risks for multi-instance deployments.The script starts gopls listening on a hardcoded
/tmp/go-pls-pipe-testsocket without waiting for it to be ready or checking for errors. If gopls fails to start, the generic-external-provider will still launch, leading to runtime failures. Additionally,/tmpis inappropriate for production use (subject to cleanup, permission issues) and will conflict if multiple containers run on the same host or if the container/pod model changes (e.g., Kubernetes).Consider:
- Making the socket path configurable (e.g., via environment variable)
- Adding error handling and health checks before launching the provider
- Using a persistent, writable directory suitable for production (e.g., a mounted volume or
/var/run)external-providers/generic-external-provider/Dockerfile (1)
37-37: ENTRYPOINT design is sound but could benefit from documentation.The default ENTRYPOINT points to
generic-external-providerdirectly, while the Makefile explicitly overrides it with--entrypoint /usr/local/bin/entrypoint.shfor golang-provider. This is a reasonable design (gopls startup is golang-specific), but it's implicit and could be clearer via comments or documentation.Consider adding a Dockerfile comment explaining when/why the entrypoint override is used.
provider/grpc/socket/pipe_windows.go (2)
26-28: Document intentional use of unix:// on Windows for gRPC passthrough.Add a comment to prevent future “fixes” back to pipe://.
func GetConnectionString(address string) string { - return fmt.Sprintf("passthrough:unix://%s", address) + // Intentionally use unix:// on Windows with the passthrough resolver so the raw + // target is handed to our context dialer. This is required for desired gRPC behavior. + // See DialWindowsPipePassthrough. (Semantics > syntax.) + return fmt.Sprintf("passthrough:unix://%s", address) }Based on learnings.
41-43: Consider exposing PipeConfig/SDDL or documenting access semantics.Default PipeConfig may restrict cross-user access; some deployments may need an explicit SDDL. Optionally accept a PipeConfig or env-based SDDL, defaulting to current behavior.
Would you like a small helper (functional option or env var) to pass SecurityDescriptor without widening default permissions?
provider/server.go (2)
103-111: Unix socket startup: remove stale path before Listen.Listening on an existing UDS file fails; pre-remove it.
Apply:
if s.Port != 0 { listen, err = net.Listen("tcp", fmt.Sprintf(":%d", s.Port)) } else if s.SocketPath != "" { + // Best-effort remove any stale socket file + _ = os.Remove(s.SocketPath) listen, err = socket.Listen(s.SocketPath) } else { return fmt.Errorf("unable to start server no serving information present") }
145-146: Graceful shutdown on ctx.Done.Start currently ignores ctx; hook it to stop gRPC and close the listener to match the contract.
Apply:
s.Log.Info(fmt.Sprintf("server listening at %v", listen.Addr())) + go func() { + <-ctx.Done() + s.Log.Info("server context canceled; shutting down") + gs.GracefulStop() + _ = listen.Close() + if s.SocketPath != "" { + _ = os.Remove(s.SocketPath) + } + }() if err := gs.Serve(listen); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
provider/internal/grpc/library.pb.gois excluded by!**/*.pb.goprovider/internal/grpc/library_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
Makefile(2 hunks)external-providers/generic-external-provider/Dockerfile(1 hunks)external-providers/generic-external-provider/entrypoint.sh(1 hunks)lsp/base_service_client/base_service_client.go(2 hunks)provider/grpc/socket/pipe_windows.go(1 hunks)provider/grpc/socket/uds.go(1 hunks)provider/internal/grpc/library.proto(2 hunks)provider/server.go(10 hunks)provider_pod_local_settings.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/grpc/socket/uds.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T15:28:31.377Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:22-26
Timestamp: 2025-07-28T15:28:31.377Z
Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-07-28T15:27:30.233Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:28-37
Timestamp: 2025-07-28T15:27:30.233Z
Learning: In Windows named pipe gRPC connections, the "unix://" URI scheme prefix must be used instead of a more semantically correct scheme like "pipe://" because it's required for the gRPC package to provide the desired behavior. This is an intentional design choice despite being semantically incorrect.
Applied to files:
provider/grpc/socket/pipe_windows.go
🧬 Code graph analysis (3)
lsp/base_service_client/base_service_client.go (6)
provider/provider.go (1)
RPCClient(158-162)jsonrpc2_v2/serve.go (2)
Dialer(34-37)Dial(57-64)lsp/base_service_client/cmd_dialer.go (1)
NewCmdDialer(25-55)jsonrpc2_v2/conn.go (1)
ConnectionOptions(46-60)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)lsp/base_service_client/base_handlers.go (1)
NewChainHandler(36-48)
provider/grpc/socket/pipe_windows.go (3)
provider/grpc/socket/uds.go (5)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)Listen(31-33)ConnectRPC(44-51)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/serve.go (2)
Listener(18-31)Dial(57-64)
provider/server.go (6)
provider/provider.go (1)
BaseClient(447-451)jsonrpc2_v2/serve.go (1)
Listener(18-31)provider/grpc/socket/pipe_windows.go (3)
Listen(41-43)GetAddress(19-24)ConnectRPC(55-62)provider/grpc/socket/uds.go (3)
Listen(31-33)GetAddress(16-25)ConnectRPC(44-51)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/conn.go (1)
Connection(66-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
🔇 Additional comments (6)
Makefile (1)
59-61: Naming inconsistency: redundant and conflicting--namearguments.Lines 60–61 pass
--namearguments twice: once as a podman container name and again in the provider arguments. This is confusing:
- Line 60:
--name nodejs ... --name nodejs(redundant)- Line 61:
--name python ... --name pylsp(inconsistent — container vs. provider name mismatch)Clarify the intent: are the second
--namearguments meant for the provider, or are they artifacts? If intentional, document or simplify.provider/grpc/socket/pipe_windows.go (1)
5-12: Avoid deterministic pipe names; switch to pid+time suffix and fix var casing.math/rand without seeding yields a deterministic sequence across restarts. Use non-crypto uniqueness via pid + time and follow Go camelCase.
Apply:
@@ -import ( +import ( "context" "fmt" "io" - "math/rand" + "os" + "time" "net" "strings" @@ func GetAddress(name string) (string, error) { - randInt := rand.Int() - pipe_name := fmt.Sprintf("\\\\.\\pipe\\%s-%v", name, randInt) - return pipe_name, nil + pipeName := fmt.Sprintf("\\\\.\\pipe\\%s-%d-%d", name, os.Getpid(), time.Now().UnixNano()) + return pipeName, nil }This stays non-cryptographic while eliminating deterministic collisions. Based on learnings.
Also applies to: 19-24
⛔ Skipped due to learnings
Learnt from: shawn-hurley PR: konveyor/analyzer-lsp#860 File: provider/grpc/socket/pipe_windows.go:22-26 Timestamp: 2025-07-28T15:28:31.377Z Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.lsp/base_service_client/base_service_client.go (1)
129-129: No breaking API changes found — verify review comment concerns are moot.The
provider.RPCClientinterface is properly defined with all required methods (Call, Notify, Close). All usages throughout the codebase—including inbase_service_client.goand external providers—are compatible with the interface. The Await semantics are preserved asCall()returns*jsonrpc2.AsyncCall, and error handling for notifications is maintained. No compilation failures or breaking changes exist.provider/server.go (1)
65-65: NewServer signature updated across all binaries.All call sites in external-providers (dotnet, yq, java, generic) correctly pass the socketPath argument. No test updates required.
provider_pod_local_settings.json (1)
17-19: Review comment is incorrect — "pipeName" is the correct key.The review conflates two different config structures. The file
provider_pod_local_settings.jsonis parsed byprovider/provider.go'sGetConfig()function, which unmarshals into the localprovider.Configstruct. This struct defines the field with YAML/JSON tagsyaml:"pipeName" json:"pipeName"(line 155 in provider.go).The
languageServerPipefield exists inlibrary.protoas part of the gRPC protocol's Config message, which is used for inter-process communication, not for parsing this local configuration file.The current JSON key
"pipeName"is correct and should not be changed.Likely an incorrect or invalid review comment.
provider/internal/grpc/library.proto (1)
23-23: Codegen is already complete; verify whether languageServerPipe should be propagated in the response Config.The generated Go code in
provider/internal/grpc/library.pb.gois already current with the proto definition—the struct field and getter methods exist. However, the config writer atprovider/server.go:282-286createsbuiltinRpcConfbut only setsLocationandDependencyPath. The incomingconfig.LanguageServerPipeis read (line 253-254) and used to initialize an RPC client, but it is not propagated to the responseInitResponse.BuiltinConfig. Confirm whetherLanguageServerPipeshould be included in the outgoing response config or if it is intentionally consumed only for initialization.
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
from
4be055b to
be1a5ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demo.Dockerfile (1)
20-20: ENTRYPOINT lacks service readiness checks and error handling.The ENTRYPOINT assumes Jaeger (
all-in-one-linux) is ready after a fixed 5-second sleep, which is unreliable. If the service takes longer to start or fails, the analyzer may try to connect to a non-functional Jaeger instance. Additionally, output is redirected to/dev/null, hiding logs that would help debug startup issues.Consider adding proper service readiness checks:
-ENTRYPOINT ["sh", "-c", "all-in-one-linux &> /dev/null & sleep 5 && konveyor-analyzer --enable-jaeger && curl -o traces.json http://localhost:16686/api/traces?service=analyzer-lsp"] +ENTRYPOINT ["sh", "-c", "all-in-one-linux & sleep 10 && until curl -f http://localhost:16686/status > /dev/null 2>&1; do sleep 1; done && konveyor-analyzer --enable-jaeger && curl -o traces.json http://localhost:16686/api/traces?service=analyzer-lsp"]Or consider using a health check script or Docker Compose for better orchestration.
Would you like me to create a more robust service startup pattern, perhaps using a separate health-check script or Docker Compose configuration?
♻️ Duplicate comments (9)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
190-191: Fix context leak on timeout.The cancel function from
context.WithTimeoutmust be called to release timers.Apply this diff:
- timeOutCtx, _ := context.WithTimeout(ctx, timeout) - err = p.rpc.Call(timeOutCtx, "workspace/executeCommand", wsp).Await(timeOutCtx, &refs) + timeOutCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + err = p.rpc.Call(timeOutCtx, "workspace/executeCommand", wsp).Await(timeOutCtx, &refs)lsp/base_service_client/base_handlers.go (2)
82-116: Data race + incorrect reset window in backoffTimer.This issue has already been flagged in previous reviews:
backoffTimerlacks synchronization, allowing concurrentHandle()calls to race on theretries,lastAttemptedTime, andlastDurationTimefields. Additionally,lastAttemptedTimeis not updated after each call, causing the reset window calculation to be incorrect.
145-149: Respect context cancellation; avoid blind sleep.This issue has already been flagged in previous reviews:
time.Sleep(d)blocks for up to 5 minutes even if the context is canceled or the connection drops. The sleep should be context-aware using a timer and select statement.lsp/base_service_client/std_dialer.go (2)
11-16: Fix type name in comments; remove misleading process note.-// Dialers in jsonrpc2_v2 return a ReadWriteCloser that sends and receives -// information from the server. CmdDialer functions as a both a ReadWriteCloser -// to the spawned process and as a Dialer that returns itself. +// Dialers in jsonrpc2_v2 return a ReadWriteCloser that sends and receives +// information from the server. StdDialer wraps stdin/stdout and implements +// both io.ReadWriteCloser and a Dialer that returns itself. // -// NOTE: Dial should only be called once. This is because closing CmdDialer also -// kills the underlying process +// NOTE: Dial should be called once. @@ -// CmdDialer.Dial returns itself as a CmdDialer is a ReadWriteCloser. +// Dial returns itself as StdDialer implements io.ReadWriteCloser.Also applies to: 49-52
22-23: Constructor doc string references CmdDialer; update to StdDialer.-// Create a new CmdDialer +// NewStdDialer creates a new StdDialer wrapping the provided streams.lsp/base_service_client/base_service_client.go (2)
213-216: Injected RPC has no inbound handler — known tech debt.With c.RPC provided, sc.Handle won’t process notifications unless the provided connection was created with the handler. Tracked for follow‑up; just calling it out.
Based on learnings
211-211: Fixed 5s sleep remains; tracked in follow‑up.The post‑Dial sleep is still present; issue #926 tracks replacing it with a real handshake/retry.
provider/grpc/socket/uds.go (1)
16-25: Fix TOCTOU race in socket path generation.Creating a temp file, removing it, then binding is racy. Generate a unique directory and place the socket inside.
@@ -import ( +import ( "context" "fmt" "net" "os" + "path/filepath" @@ -func GetAddress(name string) (string, error) { - f, err := os.CreateTemp("", fmt.Sprintf("provider-%v-*.sock", name)) - if err != nil { - return "", err - } - f.Close() - os.Remove(f.Name()) - return f.Name(), nil -} +func GetAddress(name string) (string, error) { + dir, err := os.MkdirTemp("", fmt.Sprintf("provider-%s-", name)) + if err != nil { + return "", err + } + // Caller is responsible for cleanup of dir/socket when done. + return filepath.Join(dir, "provider.sock"), nil +}provider/grpc/socket/pipe_windows.go (1)
45-53: Remove debug print and validate target format in custom dialer.Avoid stdout noise; return clear errors on bad inputs.
-func DialWindowsPipePassthrough(ctx context.Context, connectionString string) (net.Conn, error) { - pipeName, _ := strings.CutPrefix(connectionString, "unix://") - pipe, err := winio.DialPipeContext(ctx, pipeName) +func DialWindowsPipePassthrough(ctx context.Context, connectionString string) (net.Conn, error) { + pipeName, ok := strings.CutPrefix(connectionString, "unix://") + if !ok || pipeName == "" { + return nil, fmt.Errorf("unexpected pipe target: %q; want unix://<pipe>", connectionString) + } + pipe, err := winio.DialPipeContext(ctx, pipeName) if err != nil { return nil, err } - fmt.Printf("pipe: %#v", pipe) return pipe, nil }
🧹 Nitpick comments (11)
lsp/base_service_client/base_handlers.go (1)
77-80: Consider hashing params for large payloads.Using
string(req.Params)as part of the map key works but may be inefficient for large request payloads, both in terms of memory and map lookup performance. For requests with substantial parameters, consider hashing the params instead.Example using a hash:
import "hash/fnv" type requestKey struct { method string paramsHash uint64 } func hashParams(params []byte) uint64 { h := fnv.New64a() h.Write(params) return h.Sum64() } // In Handle(): requestKey := requestKey{ method: req.Method, paramsHash: hashParams(req.Params), }external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (2)
73-76: Remove stderr prints; use structured logging.Drop fmt.Fprintf(os.Stderr, ...) and rely on logr; also remove now‑unused os import.
@@ -import ( - "context" - "fmt" - "os" +import ( + "context" + "fmt" @@ - fmt.Fprintf(os.Stderr, "started generic provider init") + log.V(2).Info("starting generic provider init") @@ - log.Error(err, "ctor error") - fmt.Fprintf(os.Stderr, "ctor blah") + log.Error(err, "ctor error")Also applies to: 103-107
41-45: Avoid storing context.TODO() in struct.Either accept a ctx in NewGenericProvider or use context.Background(); if unused, drop the field.
lsp/base_service_client/std_dialer.go (1)
50-51: Optional: enforce single Dial.If “Dial once” is required, add a sync.Once or an atomic flag to prevent multiple Dial calls.
provider/grpc/socket/uds.go (1)
31-33: Optional: handle stale socket files and set perms if needed.If the process may restart, consider removing a stale socket before Listen and adjusting permissions post‑create (os.Chmod) per your access model.
provider/internal/grpc/library.proto (1)
23-23: Clarify languageServerPipe semantics.Add a comment stating expected format (absolute UDS path on Unix; named pipe on Windows; or connection string?). Prevent misuse.
external-providers/generic-external-provider/main.go (1)
30-30: Use explicit log level and exit cleanly (avoid panic).
- Replace Level(20) with a defined logrus level (e.g., TraceLevel) for clarity and consistency with other providers.
- Prefer os.Exit(1) over panic(1) to avoid stack traces in a normal CLI error path.
- logrusLog.SetLevel(logrus.Level(20)) + logrusLog.SetLevel(logrus.TraceLevel) @@ - log.Error(fmt.Errorf("no serving location"), "port or socket must be set.") - panic(1) + log.Error(fmt.Errorf("no serving location"), "port or socket must be set.") + os.Exit(1)Also applies to: 56-59
external-providers/yq-external-provider/main.go (1)
36-39: Exit instead of panic on invalid startup configuration.Panic emits a stack trace for a user error. Exit non-zero after logging.
- log.Error(fmt.Errorf("no serving location"), "port or socket must be set.") - panic(1) + log.Error(fmt.Errorf("no serving location"), "port or socket must be set.") + os.Exit(1)external-providers/java-external-provider/main.go (1)
41-43: Use os.Exit for expected CLI errors.Replace panic with a clean exit after logging.
- log.Error(fmt.Errorf("no serving location"), "port or socket must be set.") - panic(1) + log.Error(fmt.Errorf("no serving location"), "port or socket must be set.") + os.Exit(1)provider/server.go (2)
253-266: Use background context for long‑lived RPC connection.The RPC client outlives Init; use newCtx to avoid coupling to the request’s lifetime.
- rpc, err := socket.ConnectRPC(ctx, config.LanguageServerPipe, handler) + rpc, err := socket.ConnectRPC(newCtx, config.LanguageServerPipe, handler)Confirm jsonrpc2.Dial doesn’t retain/cancel with the passed ctx post‑connect; if it does, background context is required. Based on learnings.
103-111: Optional: handle stale UDS files on startup.When using Unix sockets, a leftover file from a crash can block Listen. Consider removing pre‑existing paths before listen and on shutdown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumprovider/internal/grpc/library.pb.gois excluded by!**/*.pb.goprovider/internal/grpc/library_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
Dockerfile(0 hunks)Dockerfile.windows(0 hunks)Makefile(2 hunks)debug/debug.Dockerfile(0 hunks)demo.Dockerfile(2 hunks)external-providers/dotnet-external-provider/main.go(3 hunks)external-providers/generic-external-provider/Dockerfile(2 hunks)external-providers/generic-external-provider/entrypoint.sh(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/generic-external-provider/main.go(4 hunks)external-providers/generic-external-provider/pkg/generic_external_provider/provider.go(1 hunks)external-providers/java-external-provider/go.mod(1 hunks)external-providers/java-external-provider/main.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(6 hunks)external-providers/yq-external-provider/go.mod(2 hunks)external-providers/yq-external-provider/main.go(3 hunks)external-providers/yq-external-provider/pkg/yq_provider/service_client.go(0 hunks)go.mod(4 hunks)jsonrpc2/backoff_handler.go(0 hunks)jsonrpc2/handler.go(0 hunks)jsonrpc2/jsonrpc2.go(0 hunks)jsonrpc2/rpcerr.go(0 hunks)jsonrpc2/stream.go(0 hunks)jsonrpc2/wire.go(0 hunks)lsp/base_service_client/base_handlers.go(2 hunks)lsp/base_service_client/base_service_client.go(4 hunks)lsp/base_service_client/std_dialer.go(1 hunks)provider/grpc/provider.go(6 hunks)provider/grpc/service_client.go(1 hunks)provider/grpc/socket/consts.go(1 hunks)provider/grpc/socket/pipe_windows.go(1 hunks)provider/grpc/socket/uds.go(1 hunks)provider/internal/grpc/library.proto(2 hunks)provider/provider.go(3 hunks)provider/server.go(9 hunks)provider_container_settings.json(3 hunks)provider_pod_local_settings.json(1 hunks)
💤 Files with no reviewable changes (10)
- jsonrpc2/jsonrpc2.go
- jsonrpc2/rpcerr.go
- Dockerfile.windows
- jsonrpc2/stream.go
- jsonrpc2/backoff_handler.go
- jsonrpc2/handler.go
- external-providers/yq-external-provider/pkg/yq_provider/service_client.go
- debug/debug.Dockerfile
- Dockerfile
- jsonrpc2/wire.go
🚧 Files skipped from review as they are similar to previous changes (12)
- external-providers/java-external-provider/pkg/java_external_provider/provider.go
- Makefile
- provider/grpc/socket/consts.go
- provider_pod_local_settings.json
- provider/grpc/service_client.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go
- go.mod
- external-providers/generic-external-provider/Dockerfile
- external-providers/generic-external-provider/entrypoint.sh
- external-providers/dotnet-external-provider/main.go
- provider_container_settings.json
- external-providers/yq-external-provider/go.mod
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
provider/server.go
📚 Learning: 2025-10-21T16:25:40.903Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: lsp/base_service_client/base_service_client.go:212-214
Timestamp: 2025-10-21T16:25:40.903Z
Learning: In `lsp/base_service_client/base_service_client.go`, when an RPC connection is provided via `c.RPC` (not nil), it is treated as already initialized and no handler setup is performed. This is known technical debt that needs to be addressed in the future to ensure LSP notifications like `textDocument/publishDiagnostics` are properly processed.
Applied to files:
lsp/base_service_client/base_service_client.go
📚 Learning: 2025-07-28T15:28:31.377Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:22-26
Timestamp: 2025-07-28T15:28:31.377Z
Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-07-28T15:27:30.233Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:28-37
Timestamp: 2025-07-28T15:27:30.233Z
Learning: In Windows named pipe gRPC connections, the "unix://" URI scheme prefix must be used instead of a more semantically correct scheme like "pipe://" because it's required for the gRPC package to provide the desired behavior. This is an intentional design choice despite being semantically incorrect.
Applied to files:
provider/grpc/socket/pipe_windows.go
🧬 Code graph analysis (12)
lsp/base_service_client/base_handlers.go (2)
jsonrpc2_v2/jsonrpc2.go (2)
Handler(60-75)ErrNotHandled(24-24)jsonrpc2_v2/messages.go (1)
Request(30-38)
external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
provider/provider.go (1)
BaseClient(450-454)
provider/provider.go (1)
jsonrpc2_v2/conn.go (1)
AsyncCall(375-381)
provider/grpc/socket/uds.go (5)
provider/grpc/socket/pipe_windows.go (5)
GetAddress(19-24)GetConnectionString(26-28)Listen(41-43)ConnectGRPC(30-39)ConnectRPC(55-66)jsonrpc2_v2/serve.go (3)
Listener(18-31)Dialer(34-37)Dial(57-64)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)jsonrpc2_v2/net.go (1)
NetDialer(62-68)
lsp/base_service_client/std_dialer.go (1)
jsonrpc2_v2/serve.go (2)
Dialer(34-37)Dial(57-64)
provider/server.go (6)
provider/provider.go (2)
BaseClient(450-454)Client(445-448)jsonrpc2_v2/serve.go (1)
Listener(18-31)provider/grpc/socket/pipe_windows.go (2)
Listen(41-43)ConnectRPC(55-66)provider/grpc/socket/uds.go (2)
Listen(31-33)ConnectRPC(44-55)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)
external-providers/generic-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
provider/grpc/provider.go (5)
provider/provider.go (1)
Config(88-98)provider/internal/grpc/library.pb.go (3)
Config(78-89)Config(102-102)Config(117-119)provider/grpc/socket/pipe_windows.go (3)
GetAddress(19-24)GetConnectionString(26-28)ConnectGRPC(30-39)provider/grpc/socket/uds.go (3)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)
external-providers/java-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
external-providers/yq-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
lsp/base_service_client/base_service_client.go (5)
provider/provider.go (1)
RPCClient(161-165)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)lsp/base_service_client/base_handlers.go (1)
NewChainHandler(36-48)lsp/base_service_client/cmd_dialer.go (1)
NewCmdDialer(25-55)jsonrpc2_v2/conn.go (1)
ConnectionOptions(46-60)
provider/grpc/socket/pipe_windows.go (5)
provider/grpc/socket/uds.go (5)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)Listen(31-33)ConnectRPC(44-55)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/serve.go (2)
Listener(18-31)Dial(57-64)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)jsonrpc2_v2/conn.go (2)
Connection(66-79)ConnectionOptions(46-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
🔇 Additional comments (20)
demo.Dockerfile (1)
15-15: Package additions are reasonable for a demo image.Adding
vimandprocpssupports interactive debugging and process introspection. Whilevimadds some image size, it's acceptable for a demo Dockerfile. No changes needed here.external-providers/java-external-provider/pkg/java_external_provider/service_client.go (5)
24-24: LGTM! Import updated to jsonrpc2_v2.The import alias maintains code compatibility while migrating to the newer API.
72-72: LGTM! Simplified condition is correct.The explicit nil check is unnecessary since
len(nil)returns 0 in Go.
254-254: LGTM! RPC call migrated correctly to Await pattern.The call properly uses the passed-in context and maintains existing error handling.
312-312: LGTM! Shutdown RPC call migrated correctly.The call uses the properly managed
shutdownCtxwith its cancel deferred at line 295.
411-412: LGTM! Initialize RPC call migrated correctly.The call properly uses
.Await()within the retry loop and maintains existing error handling logic.external-providers/java-external-provider/go.mod (3)
24-24: Cross-platform socket support dependency confirmed.The addition of
github.com/Microsoft/go-winio v0.6.2aligns well with the PR's socket communication objectives. This dependency is essential for named-pipe support on Windows, enabling cross-platform UDS/named-pipe compatibility alongside the Unix domain socket support already available on Linux/macOS.
8-49: Coordinated dependency updates look appropriate.The updates across otel packages (v1.34.0 → v1.35.0), golang.org utilities (net, sys, text), and other transitive dependencies are conservative patch/minor-level bumps. The changes are consistent with the broader socket communication infrastructure being added in this PR.
13-13: gRPC indirect marking is correct and intentional.Verification confirms:
- No direct
google.golang.org/grpcimports in java-external-provider source files- gRPC is transitively required through
github.com/konveyor/analyzer-lsp(a direct dependency listed in therequireblock)- The
// indirectcomment correctly indicates the dependency is not directly imported by this providerlsp/base_service_client/base_handlers.go (1)
3-11: LGTM!The new imports appropriately support the BackoffHandler's exponential backoff calculation, synchronization, and timing requirements.
external-providers/generic-external-provider/go.mod (2)
16-16: Addition of go-winio aligns with socket communication feature.The new dependency
github.com/Microsoft/go-winio v0.6.2is appropriate for supporting cross-platform socket communication (Windows named pipes). This addition directly supports the PR objective of adding socket-based communication across platforms.
27-37: Version compatibility and security advisories verified—no critical issues found.The updated dependencies are indeed coordinated from the root analyzer-lsp module through the
replacedirective. Verification confirms:
- All versions are compatible (go mod tidy succeeds with no errors)
- No known security vulnerabilities (CVEs) in gRPC v1.73.0, protobuf v1.36.6, or OpenTelemetry v1.35.0
- Deprecated packages (Jaeger exporter v1.17.0, golang/protobuf v1.5.4) are indirect dependencies only
The current versions are stable and reasonably recent; while newer versions exist (OpenTelemetry v1.38.0, gRPC v1.76.0, protobuf v1.36.10), they offer no security advantage for this codebase.
external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
33-33: API change from concrete type to interface is safe — no callers require concrete type.Verification confirms all 5 callers (main.go, and 4 test files) use the returned
provider.BaseClientinterface without assuming or casting to the concrete*genericProvidertype. No type assertions or reflection on the concrete type were found.provider/provider.go (2)
92-92: Config.UseSocket: clarify precedence with Address.If both Address and UseSocket/pipe are set, what wins? Recommend documenting/validating to avoid ambiguous startup.
162-165: RPCClient interface changes are properly implemented.Verification confirms that the Connection type in jsonrpc2_v2/conn.go fully implements the RPCClient interface with all required methods and compatible signatures:
Call(line 315) returns*AsyncCall, which resolves to*jsonrpc2.AsyncCallvia the package aliasNotify(line 263) implements the required signatureClose(line 495) implements the required signatureConnection is returned from
jsonrpc2.Dial()calls (e.g., uds.go:50, pipe_windows.go:61) where RPCClient is expected. No other implementers were found that require updating, and the code compiles without errors.provider/grpc/socket/pipe_windows.go (1)
5-17: Seed randomness for pipe name generation to avoid deterministic collisions.math/rand’s global RNG is unseeded; first values repeat across runs. Seed or use a local RNG.
import ( "context" "fmt" "io" + "time" "math/rand" "net" "strings" @@ func GetAddress(name string) (string, error) { - randInt := rand.Int() + r := rand.New(rand.NewSource(time.Now().UnixNano())) + randInt := r.Int() pipe_name := fmt.Sprintf("\\\\.\\pipe\\%s-%v", name, randInt) return pipe_name, nil }Also applies to: 19-24
⛔ Skipped due to learnings
Learnt from: shawn-hurley PR: konveyor/analyzer-lsp#860 File: provider/grpc/socket/pipe_windows.go:22-26 Timestamp: 2025-07-28T15:28:31.377Z Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.provider/grpc/provider.go (4)
14-14: LGTM!The socket package import is necessary for the new socket-based communication functionality.
44-44: LGTM!Logger propagation to the
startfunction improves observability and debugging capabilities.
194-206: LGTM with observation.The logging of provider configuration improves debuggability. The field names match the protobuf definitions.
Note: The typo "Initalized" (should be "Initialized") exists in the protobuf definition (provider/internal/grpc/library.pb.go line 85), not introduced here.
347-353: LGTM!Extracting
LogProviderOutas a standalone function with an explicit logger parameter improves modularity and testability.
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
from
be1a5ca to
e60778e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (16)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
497-506: Critical: Cleanup missing on Dial failure—orphaned LSP process and open pipes.When
dialercreation orjsonrpc2.Dialfails, the spawned child process (lines 462-488) continues running and stdin/stdout pipes remain open, leaking resources.Apply this diff to ensure cleanup on error:
// Create a shared input,ouput dialer dialer := base.NewStdDialer(stdin, stdout) rpc, err := jsonrpc2.Dial(ctx, dialer, jsonrpc2.ConnectionOptions{ Handler: base.NewChainHandler(base.LogHandler(log)), }) if err != nil { log.Error(err, "unable to connect over new package") + cancelFunc() + _ = cmd.Process.Kill() return nil, additionalBuiltinConfig, err }provider/provider.go (1)
155-158: Major: Typo in public API field "Initalized" will leak to YAML/JSON.The field name and struct tags are misspelled as
Initalized(should beInitialized). This becomes part of the public configuration API and will cause confusion.Apply this diff to fix:
PipeName string `yaml:"pipeName" json:"pipeName"` // Given a pipe name for the init config, we will use that pipe and connect to an already inited provider. -Initalized bool `yaml:"initalized" json:"initalized"` +Initialized bool `yaml:"initialized" json:"initialized"`Then update all usages (e.g., in provider/server.go) to reference
.Initialized.lsp/base_service_client/std_dialer.go (3)
11-16: Minor: Documentation incorrectly references "CmdDialer" instead of "StdDialer".The type comment references "CmdDialer" three times (lines 12, 15) but the actual type is
StdDialer. Additionally, the comment claims "closing CmdDialer also kills the underlying process," but this implementation simply wraps existing I/O streams without managing any process.Apply this diff:
-// Dialers in jsonrpc2_v2 return a ReadWriteCloser that sends and receives -// information from the server. CmdDialer functions as a both a ReadWriteCloser -// to the spawned process and as a Dialer that returns itself. +// StdDialer wraps stdin/stdout streams and implements both io.ReadWriteCloser +// and a Dialer interface that returns itself. // -// NOTE: Dial should only be called once. This is because closing CmdDialer also -// kills the underlying process +// NOTE: Dial should only be called once. type StdDialer struct {
22-30: Minor: Documentation references incorrect type name.Line 22 says "Create a new CmdDialer" but should reference "StdDialer".
Apply this diff:
-// Create a new CmdDialer +// NewStdDialer creates a new StdDialer wrapping the provided stdin/stdout streams. func NewStdDialer(stdin io.WriteCloser, stdout io.ReadCloser) jsonrpc2.Dialer {
49-52: Minor: Documentation references incorrect type name.Line 49 says "CmdDialer.Dial" but should reference "StdDialer".
Apply this diff:
-// CmdDialer.Dial returns itself as a CmdDialer is a ReadWriteCloser. +// Dial returns itself as a StdDialer is a ReadWriteCloser. func (rwc *StdDialer) Dial(ctx context.Context) (io.ReadWriteCloser, error) {demo.Dockerfile (1)
1-2: Critical: Local-only base image breaks reproducibility.The base image
localhost/testing/analyzer-lsp:testing-socketreferences a local image that won't exist in CI/CD pipelines or when the repository is cloned elsewhere, preventing the demo image from being built.Either:
- Push to a public registry and update:
quay.io/konveyor/analyzer-lsp:testing-socket- Revert to the original:
quay.io/konveyor/analyzer-lsp- Remove this change if it's temporary development work
-#FROM quay.io/konveyor/analyzer-lsp -FROM localhost/testing/analyzer-lsp:testing-socket +FROM quay.io/konveyor/analyzer-lsp:testing-socketexternal-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
190-191: Fix context leak on timeout.The context created with
context.WithTimeoutis not canceled, which leaks timers and resources.Apply this diff to capture and defer the cancel function:
- timeOutCtx, _ := context.WithTimeout(ctx, timeout) - err = p.rpc.Call(timeOutCtx, "workspace/executeCommand", wsp).Await(timeOutCtx, &refs) + timeOutCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + err = p.rpc.Call(timeOutCtx, "workspace/executeCommand", wsp).Await(timeOutCtx, &refs)provider/grpc/socket/pipe_windows.go (1)
45-53: Remove debug print and add error handling.The function has two issues:
- Line 51: Debug print statement should be removed from production code
- Line 46: No validation when
CutPrefixfails or whenpipeNameis emptyApply this diff to improve error handling and remove debug output:
func DialWindowsPipePassthrough(ctx context.Context, connectionString string) (net.Conn, error) { - pipeName, _ := strings.CutPrefix(connectionString, "unix://") + pipeName, ok := strings.CutPrefix(connectionString, "unix://") + if !ok { + return nil, fmt.Errorf("unexpected connection string for windows pipe: %q; expected unix://<pipe>", connectionString) + } + if pipeName == "" { + return nil, fmt.Errorf("empty pipe name in connection string: %q", connectionString) + } pipe, err := winio.DialPipeContext(ctx, pipeName) if err != nil { return nil, err } - fmt.Printf("pipe: %#v", pipe) return pipe, nil }provider/grpc/socket/uds.go (1)
16-24: Race condition in socket address generation.Creating a temporary file, removing it, and returning the path creates a window where another process could create a file or socket with the same name before this socket is bound.
Consider generating a unique name without creating/removing the file:
func GetAddress(name string) (string, error) { - f, err := os.CreateTemp("", fmt.Sprintf("provider-%v-*.sock", name)) - if err != nil { - return "", err - } - f.Close() - os.Remove(f.Name()) - return f.Name(), nil + tempDir := os.TempDir() + randSuffix := make([]byte, 8) + if _, err := rand.Read(randSuffix); err != nil { + return "", fmt.Errorf("failed to generate random suffix: %w", err) + } + sockPath := filepath.Join(tempDir, fmt.Sprintf("provider-%s-%x.sock", name, randSuffix)) + return sockPath, nil }Don't forget to add the necessary imports:
import ( + "crypto/rand" "context" + "encoding/hex" "fmt" "net" "os" + "path/filepath"provider/grpc/provider.go (2)
264-303: Critical: Socket connection logic fails for TCP addresses.The current implementation unconditionally calls
socket.ConnectGRPC(connectionString)on line 298 for both socket-based and TCP-based connections. However,socket.ConnectGRPCis designed for socket connections (using custom dialers on Windows) and may not handle raw TCP addresses like"localhost:1234"correctly.Apply this diff to fix the connection logic and related issues:
connectionString = fmt.Sprintf("localhost:%v", port) cmd = exec.CommandContext(ctx, config.BinaryPath, "--port", fmt.Sprintf("%v", port), "--name", name) } // TODO: For each output line, log that line here, allows the server's to output to the main log file. Make sure we name this correctly // cmd will exit with the ending of the ctx. out, err := cmd.StdoutPipe() if err != nil { return nil, nil, err } - fmt.Printf("\ncommand: %v\n", cmd) + log.V(3).Info("starting provider binary", "command", cmd.String()) if out != nil { go LogProviderOut(context.Background(), out, log) } err = cmd.Start() if err != nil { return nil, nil, err } - conn, err := socket.ConnectGRPC(connectionString) + var conn *grpc.ClientConn + if config.UseSocket { + conn, err = socket.ConnectGRPC(connectionString) + } else { + conn, err = grpc.NewClient(connectionString, + grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(socket.MAX_MESSAGE_SIZE)), + grpc.WithTransportCredentials(insecure.NewCredentials()), + ) + } if err != nil { log.Error(err, "did not connect") + return nil, nil, err } return conn, out, nilAdditional fixes:
- Line 288: Replace debug
fmt.Printfwith structured logging- Line 301-302: Add explicit error return after logging connection failure
317-342: Fix error handling in TLS connection paths.Both TLS connection paths (with and without JWT) log errors but still return
conn, nil, nilon failure instead of propagating the error. This causes connection failures to be silently ignored.Apply this diff:
if config.JWTToken == "" { conn, err := grpc.NewClient(fmt.Sprintf(config.Address), grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(socket.MAX_MESSAGE_SIZE)), grpc.WithTransportCredentials(creds)) if err != nil { log.Error(err, "did not connect") + return nil, nil, err } return conn, nil, nil } else { i := &jwtTokeInterceptor{ Token: config.JWTToken, } conn, err := grpc.NewClient(fmt.Sprintf(config.Address), grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(socket.MAX_MESSAGE_SIZE)), grpc.WithTransportCredentials(creds), grpc.WithUnaryInterceptor(i.unaryInterceptor)) if err != nil { log.Error(err, "did not connect") + return nil, nil, err } return conn, nil, nil }lsp/base_service_client/base_handlers.go (5)
118-126: Fix misleading comment block.Docs reference a logging handler, not this backoff handler.
Apply:
-// Log is invoked for all messages flowing through a Conn. -// direction indicates if the message being received or sent -// id is the message id, if not set it was a notification -// elapsed is the time between a call being seen and the response, and is -// negative for anything that is not a response. -// method is the method name specified in the message -// payload is the parameters for a call or notification, and the result for a -// response -// Request is called near the start of processing any request. +// Handle applies exponential backoff per (method, params) key. +// It computes a delay (cap 5m), sleeps with context awareness, then returns +// jsonrpc2.ErrNotHandled to continue the chain. Backoff resets after ~1m idle.
82-86: Fix data race in backoffTimer and correct reset window.Concurrent Handle() calls can race on retries/last* fields; lastAttemptedTime isn’t updated per call, so the 1m reset window is wrong. Lock the timer and record now every time.
Apply:
type backoffTimer struct { - retries float64 - lastAttemptedTime *time.Time - lastDurationTime time.Duration + mu sync.Mutex + retries float64 + lastAttemptedTime *time.Time + lastDurationTime time.Duration } func (b *backoffTimer) BackoffRequest() time.Duration { - if b.lastAttemptedTime == nil { - t := time.Now() - b.lastAttemptedTime = &t - b.lastDurationTime = time.Duration(0) - return b.lastDurationTime - } + b.mu.Lock() + defer b.mu.Unlock() + now := time.Now() + if b.lastAttemptedTime == nil { + b.lastAttemptedTime = &now + b.lastDurationTime = 0 + return 0 + } // if backoff exists but has been more than a minute passed the // the last back off, then reset the backoff. - if time.Now().After(b.lastAttemptedTime.Add(b.lastDurationTime).Add(time.Minute * 1)) { + if now.After(b.lastAttemptedTime.Add(b.lastDurationTime).Add(time.Minute)) { b.retries = 0 - t := time.Now() - b.lastAttemptedTime = &t - b.lastDurationTime = time.Duration(0) - return b.lastDurationTime + b.lastAttemptedTime = &now + b.lastDurationTime = 0 + return 0 } // calculated back off - b.lastDurationTime = time.Second * time.Duration((math.Pow(2, b.retries))) + b.lastDurationTime = time.Second * time.Duration(math.Pow(2, b.retries)) // Cap back off at 5 min. if b.lastDurationTime >= (time.Minute * 5) { b.lastDurationTime = time.Minute * 5 } - b.retries = b.retries + 1 + b.retries++ + b.lastAttemptedTime = &now return b.lastDurationTime }Also applies to: 88-116
145-149: Honor context cancellation; replace blind sleep.time.Sleep(d) can block up to 5m even after ctx.Done().
Apply:
- d := backOff.BackoffRequest() - b.logger.V(9).Info("starting backing off request", "method", req.Method, "duration", d) - time.Sleep(d) - b.logger.V(9).Info("stopping backing off request", "method", req.Method) - return nil, jsonrpc2.ErrNotHandled + d := backOff.BackoffRequest() + if d > 0 { + b.logger.V(9).Info("starting backoff", "method", req.Method, "duration", d) + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-ctx.Done(): + b.logger.V(9).Info("backoff canceled", "method", req.Method) + return nil, jsonrpc2.ErrNotHandled + case <-timer.C: + } + b.logger.V(9).Info("finished backoff", "method", req.Method) + } + return nil, jsonrpc2.ErrNotHandled
63-75: Prevent unbounded growth of failedRequests.Entries are never evicted; memory can grow without bound.
Suggested lazy, low-risk cleanup:
func NewBackoffHandler(log logr.Logger) jsonrpc2.Handler { - return &BackoffHandler{ + h := &BackoffHandler{ failedRequests: make(map[requestKey]*backoffTimer), failedRequestsMu: sync.Mutex{}, logger: log, } + // Optional: background pruning every 5m; make stoppable if you add a Close(). + go h.cleanupOldEntries() + return h -} +} + +func (b *BackoffHandler) cleanupOldEntries() { + t := time.NewTicker(5 * time.Minute) + defer t.Stop() + for range t.C { + cutoff := time.Now().Add(-10 * time.Minute) + b.failedRequestsMu.Lock() + for k, v := range b.failedRequests { + if v.lastAttemptedTime != nil && v.lastAttemptedTime.Before(cutoff) { + delete(b.failedRequests, k) + } + } + b.failedRequestsMu.Unlock() + } +}Alternatively, use a bounded LRU/TTL cache.
133-149: Backoff throttles all requests, not just failures.A timer entry is created on first sight and backoff increases on every subsequent call, even if downstream succeeds. This unnecessarily delays healthy traffic.
Minimal behavior fix: only back off when a prior failure was recorded; don’t auto-create entries here.
- b.failedRequestsMu.Lock() - backOff, ok := b.failedRequests[requestKey] - if !ok { - backOff = &backoffTimer{retries: 0, lastAttemptedTime: nil, lastDurationTime: 0} - b.failedRequests[requestKey] = backOff - } - b.failedRequestsMu.Unlock() + b.failedRequestsMu.Lock() + backOff, ok := b.failedRequests[requestKey] + b.failedRequestsMu.Unlock() + if !ok { + // No prior failure recorded -> no backoff + return nil, jsonrpc2.ErrNotHandled + }Add recording APIs and call them at failure/success sites:
// New: record a failure func (b *BackoffHandler) RecordFailure(method string, params []byte) { key := requestKey{method: method, params: string(params)} b.failedRequestsMu.Lock() defer b.failedRequestsMu.Unlock() if _, ok := b.failedRequests[key]; !ok { b.failedRequests[key] = &backoffTimer{} } } // New: reset on success func (b *BackoffHandler) Reset(method string, params []byte) { key := requestKey{method: method, params: string(params)} b.failedRequestsMu.Lock() delete(b.failedRequests, key) b.failedRequestsMu.Unlock() }If changing constructor type is disruptive, add a parallel constructor returning *BackoffHandler for wiring these calls.
Run to locate call sites to wire failure/success hooks:
#!/bin/bash rg -nP -C3 'NewBackoffHandler\(|BackoffHandler\)'
🧹 Nitpick comments (3)
provider/grpc/socket/consts.go (1)
1-5: Optional: Consider idiomatic Go naming convention.The constant
MAX_MESSAGE_SIZE(8MB) is functionally correct, but Go convention typically usesMaxMessageSizefor exported constants rather than all-caps.If you prefer idiomatic Go style, apply this diff:
package socket const ( - MAX_MESSAGE_SIZE = 1024 * 1024 * 8 + MaxMessageSize = 1024 * 1024 * 8 )Then update references in
pipe_windows.go,uds.go, andprovider/server.goaccordingly.lsp/base_service_client/base_handlers.go (2)
77-80: Reduce key cardinality and copies for params.string(req.Params) copies entire payload and varies with non-canonical JSON. Consider hashing canonical params or keying by method only (if sufficient).
Example:
func makeKey(method string, params json.RawMessage) requestKey { sum := sha256.Sum256(params) // or canonicalize first return requestKey{method: method, params: hex.EncodeToString(sum[:])} }
106-115: Consider adding jitter to backoff.Deterministic 2^n can cause lockstep retries. Apply ±50% jitter to spread load.
Example:
base := time.Second * time.Duration(math.Pow(2, b.retries)) jitter := time.Duration(rand.Int63n(int64(base))) - base/2 b.lastDurationTime = base + jitterSeed rand safely at init.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumprovider/internal/grpc/library.pb.gois excluded by!**/*.pb.goprovider/internal/grpc/library_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
Dockerfile(0 hunks)Dockerfile.windows(0 hunks)Makefile(2 hunks)debug/debug.Dockerfile(0 hunks)demo.Dockerfile(2 hunks)external-providers/dotnet-external-provider/main.go(3 hunks)external-providers/generic-external-provider/Dockerfile(2 hunks)external-providers/generic-external-provider/entrypoint.sh(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/generic-external-provider/main.go(4 hunks)external-providers/generic-external-provider/pkg/generic_external_provider/provider.go(1 hunks)external-providers/java-external-provider/go.mod(1 hunks)external-providers/java-external-provider/main.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(6 hunks)external-providers/yq-external-provider/go.mod(2 hunks)external-providers/yq-external-provider/main.go(3 hunks)external-providers/yq-external-provider/pkg/yq_provider/service_client.go(0 hunks)go.mod(4 hunks)jsonrpc2/backoff_handler.go(0 hunks)jsonrpc2/handler.go(0 hunks)jsonrpc2/jsonrpc2.go(0 hunks)jsonrpc2/rpcerr.go(0 hunks)jsonrpc2/stream.go(0 hunks)jsonrpc2/wire.go(0 hunks)lsp/base_service_client/base_handlers.go(2 hunks)lsp/base_service_client/base_service_client.go(4 hunks)lsp/base_service_client/std_dialer.go(1 hunks)provider/grpc/provider.go(6 hunks)provider/grpc/service_client.go(1 hunks)provider/grpc/socket/consts.go(1 hunks)provider/grpc/socket/pipe_windows.go(1 hunks)provider/grpc/socket/uds.go(1 hunks)provider/internal/grpc/library.proto(2 hunks)provider/provider.go(3 hunks)provider/server.go(9 hunks)provider_container_settings.json(3 hunks)provider_pod_local_settings.json(1 hunks)
💤 Files with no reviewable changes (10)
- jsonrpc2/jsonrpc2.go
- jsonrpc2/rpcerr.go
- jsonrpc2/wire.go
- Dockerfile.windows
- debug/debug.Dockerfile
- jsonrpc2/backoff_handler.go
- external-providers/yq-external-provider/pkg/yq_provider/service_client.go
- jsonrpc2/stream.go
- Dockerfile
- jsonrpc2/handler.go
🚧 Files skipped from review as they are similar to previous changes (9)
- provider/internal/grpc/library.proto
- provider/grpc/service_client.go
- external-providers/yq-external-provider/main.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go
- external-providers/generic-external-provider/entrypoint.sh
- lsp/base_service_client/base_service_client.go
- provider_container_settings.json
- external-providers/java-external-provider/go.mod
- external-providers/generic-external-provider/Dockerfile
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-28T15:28:31.377Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:22-26
Timestamp: 2025-07-28T15:28:31.377Z
Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-07-28T15:27:30.233Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:28-37
Timestamp: 2025-07-28T15:27:30.233Z
Learning: In Windows named pipe gRPC connections, the "unix://" URI scheme prefix must be used instead of a more semantically correct scheme like "pipe://" because it's required for the gRPC package to provide the desired behavior. This is an intentional design choice despite being semantically incorrect.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
provider/server.go
🧬 Code graph analysis (12)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (5)
lsp/base_service_client/std_dialer.go (1)
NewStdDialer(23-30)jsonrpc2_v2/serve.go (1)
Dial(57-64)jsonrpc2_v2/conn.go (1)
ConnectionOptions(46-60)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)lsp/base_service_client/base_handlers.go (2)
NewChainHandler(36-48)LogHandler(22-27)
external-providers/java-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
provider/provider.go (1)
BaseClient(450-454)
provider/grpc/socket/uds.go (4)
provider/grpc/socket/pipe_windows.go (5)
GetAddress(19-24)GetConnectionString(26-28)Listen(41-43)ConnectGRPC(30-39)ConnectRPC(55-66)jsonrpc2_v2/serve.go (3)
Listener(18-31)Dialer(34-37)Dial(57-64)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/net.go (1)
NetDialer(62-68)
lsp/base_service_client/base_handlers.go (2)
jsonrpc2_v2/jsonrpc2.go (2)
Handler(60-75)ErrNotHandled(24-24)jsonrpc2_v2/messages.go (1)
Request(30-38)
provider/grpc/socket/pipe_windows.go (5)
provider/grpc/socket/uds.go (5)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)Listen(31-33)ConnectRPC(44-55)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/serve.go (2)
Listener(18-31)Dial(57-64)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)jsonrpc2_v2/conn.go (2)
Connection(66-79)ConnectionOptions(46-60)
external-providers/dotnet-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
external-providers/generic-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
lsp/base_service_client/std_dialer.go (1)
jsonrpc2_v2/serve.go (2)
Dialer(34-37)Dial(57-64)
provider/provider.go (1)
jsonrpc2_v2/conn.go (1)
AsyncCall(375-381)
provider/grpc/provider.go (5)
provider/provider.go (1)
Config(88-98)provider/internal/grpc/library.pb.go (3)
Config(78-89)Config(102-102)Config(117-119)provider/grpc/socket/pipe_windows.go (3)
GetAddress(19-24)GetConnectionString(26-28)ConnectGRPC(30-39)provider/grpc/socket/uds.go (3)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)
provider/server.go (5)
provider/provider.go (2)
BaseClient(450-454)Client(445-448)provider/grpc/socket/pipe_windows.go (2)
Listen(41-43)ConnectRPC(55-66)provider/grpc/socket/uds.go (2)
Listen(31-33)ConnectRPC(44-55)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test
- GitHub Check: test (windows-latest)
🔇 Additional comments (26)
external-providers/yq-external-provider/go.mod (1)
16-41: LGTM—Routine dependency updates aligned with cross-platform socket support.The addition of
github.com/Microsoft/go-winioenables Windows named pipe support, and the version bumps for OpenTelemetry, gRPC, and related packages are standard maintenance.go.mod (1)
6-55: LGTM—Dependency updates enable cross-platform socket communication.The new direct dependency
github.com/Microsoft/go-winio v0.6.2supports Windows named pipes, aligning with the PR's cross-platform socket objectives. The version bumps are routine maintenance.external-providers/java-external-provider/pkg/java_external_provider/provider.go (2)
22-23: LGTM—Import updates align with jsonrpc2_v2 migration.The import path change to
jsonrpc2_v2and thebasealias support the new dialer-based RPC connection mechanism introduced in this PR.
477-477: LGTM—Improved error visibility during language server retry.Adding the error log when retrying language server startup enhances observability.
provider/provider.go (3)
18-18: LGTM—Import supports jsonrpc2_v2 migration.The import path update aligns with the new async RPC pattern and socket-based transport introduced in this PR.
92-92: LGTM—New UseSocket field enables socket-based provider communication.The boolean flag allows configuration of socket-based vs network-based provider connections.
162-165: LGTM—RPCClient interface updated for async pattern and explicit lifecycle.The interface changes support the new jsonrpc2_v2 transport:
Callnow returns*jsonrpc2.AsyncCallfor async operationsClose()method added for explicit cleanup- Parameter type updated from
interface{}toany(idiomatic Go 1.18+)These are intentional breaking changes for the new socket-based RPC mechanism.
lsp/base_service_client/std_dialer.go (1)
32-47: LGTM—Clean I/O delegation and proper error handling.The Read, Write, and Close methods correctly delegate to the underlying streams, and Close properly combines errors from both streams.
demo.Dockerfile (1)
15-15: LGTM—Additional debugging tools added.Installing
vimandprocpsprovides useful debugging capabilities within the demo container.provider_pod_local_settings.json (1)
17-18: LGTM—Pipe configuration enables language server communication testing.The
pipeNamefield aligns with the newInitConfig.PipeNamecapability. Note that the Unix socket path/tmp/go-pls-pipe-testis platform-specific and won't work on Windows; consider documenting this if this config is reused across platforms.external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
33-33: LGTM! Interface return type improves abstraction.The change from returning a concrete
*genericProviderto theprovider.BaseClientinterface is well-aligned with the socket communication feature, enabling polymorphic usage across different transport mechanisms.external-providers/java-external-provider/pkg/java_external_provider/service_client.go (3)
24-24: LGTM! Import updated for jsonrpc2_v2 migration.The import change aligns with the broader refactoring to use the updated jsonrpc2 implementation across providers.
72-74: LGTM! Simplified condition is correct.The condition simplification maintains the same behavior while being more concise.
191-200: LGTM! RPC calls updated to use Await pattern.The transition to the
.Await()pattern is consistent with the jsonrpc2_v2 API and maintains proper error handling withIsRPCClosedchecks.Also applies to: 254-262, 312-316, 411-420
external-providers/dotnet-external-provider/main.go (1)
21-21: LGTM! Socket support added consistently.The addition of the
--socketflag, updated validation to require either port or socket, and the extendedNewServercall are well-aligned with the cross-platform socket communication feature.Also applies to: 38-40, 59-59
external-providers/generic-external-provider/main.go (1)
17-17: LGTM! Socket support added consistently.The socket flag, validation logic, and
NewServercall updates align well with the socket communication feature.Also applies to: 56-59, 77-77
external-providers/java-external-provider/main.go (1)
17-17: LGTM! Socket support added consistently.The socket flag, validation, and
NewServercall updates are well-aligned with the cross-platform socket communication feature.Also applies to: 41-43, 61-61
external-providers/generic-external-provider/go.mod (1)
16-16: LGTM! Dependency updates support socket communication.The addition of
go-winiofor Windows named pipe support and the OpenTelemetry/gRPC version bumps align with the socket communication feature requirements.Also applies to: 27-37
provider/grpc/socket/uds.go (1)
27-33: LGTM! Socket connection functions implemented correctly.The
GetConnectionString,Listen,ConnectGRPC, andConnectRPCfunctions properly handle Unix domain socket connections for both gRPC and JSON-RPC transports.Also applies to: 35-55
provider/grpc/socket/pipe_windows.go (1)
19-24: LGTM! Windows pipe support implemented correctly.The Windows named pipe functions properly handle pipe address generation, connection string formatting, gRPC client creation, listening, and JSON-RPC connections. The use of "unix://" in the connection string is intentional per project requirements.
Based on learnings
Also applies to: 26-28, 30-43, 55-78
provider/server.go (2)
103-111: LGTM: Socket and TCP listener setup.The conditional logic correctly handles both TCP port and socket-based listening, with appropriate error handling when neither is configured.
509-511: LGTM: JWT callback signature updated.The return type change from
interface{}toanyis a standard Go 1.18+ modernization, asanyis an alias forinterface{}.provider/grpc/provider.go (3)
194-207: LGTM: Configuration logging added.Logging the provider configuration helps with debugging and troubleshooting. The new fields
LanguageServerPipeandInitalizedare properly wired into the protobuf config.
306-315: LGTM: Insecure address-based connection.The insecure connection path correctly uses
grpc.NewClientwith appropriate message size limits and properly returns errors.
347-353: LGTM: Logging improvements.Converting
LogProviderOutto a standalone function with explicit logger parameter and using structured logging (log.V(3).Info) is a good improvement for maintainability.Makefile (1)
59-59: Entrypoint script verified—no issues found.The entrypoint script correctly forwards CLI arguments to the provider binary using
"$@", and the Dockerfile confirms the script is copied to/usr/local/bin/entrypoint.shduring the build.The entrypoint is specific to the golang-provider because it starts the Go Language Server (gopls) on a unix socket before launching the provider—a setup that nodejs and python variants do not require. The selective application to only golang-provider is intentional and correct.
Signed-off-by: Shawn Hurley <[email protected]>
* Adding winio from microsoft to use named pipes * Fixup to use the non-deprecated way of creating clients * Adding package socket, to encapsulate getting sockets and using them. Allows for build parameters to be used so that the right functions are called on the correct platforms. Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
* Adding ability for client and providers to handle new GRPC config field `LanguageServerPipe`. * When this is set, the base server will create a connection to the pipe and set in the init config the RPC field. * Generic Provider and java provider will use this RPC field when set. * Updated demo with an example of usage for golang provider. Signed-off-by: Shawn Hurley <[email protected]>
* Fixed up some things that code rabbit found * added ability for generic service client to continue to have handlers even when provider/server creates the connection Signed-off-by: Shawn Hurley <[email protected]>
shawn-hurley
force-pushed
the
feature/adding-socket-communication
branch
from
e60778e to
80be3a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
73-73: Remove or replace debug statements with structured logging.The
fmt.Fprintf(os.Stderr, ...)statements bypass the providedlogr.Logger. Line 105's message "ctor blah" appears to be temporary debug code.Consider using the logger parameter for consistency:
- fmt.Fprintf(os.Stderr, "started generic provider init") + log.V(1).Info("started generic provider init")if err != nil { log.Error(err, "ctor error") - fmt.Fprintf(os.Stderr, "ctor blah") return nil, provider.InitConfig{}, fmt.Errorf("ctor error: %w", err) }Also applies to: 105-105
provider/server.go (1)
509-519: Enforce JWT signing method to avoid alg confusion.Verify expected HMAC method before returning the key.
Apply:
- token, err := jwt.Parse(tokenString, func(t *jwt.Token) (any, error) { - return []byte(s.SecretKey), nil - }) + token, err := jwt.Parse(tokenString, func(t *jwt.Token) (any, error) { + if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { + return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) + } + return []byte(s.SecretKey), nil + })
♻️ Duplicate comments (8)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
190-191: Fix context leak on timeout.The cancel function from
context.WithTimeoutmust be captured and called to prevent timer leaks. This issue was previously flagged and remains unresolved.Apply this diff to fix the leak:
-timeOutCtx, _ := context.WithTimeout(ctx, timeout) +timeOutCtx, cancel := context.WithTimeout(ctx, timeout) +defer cancel() err = p.rpc.Call(timeOutCtx, "workspace/executeCommand", wsp).Await(timeOutCtx, &refs)external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
497-507: Critical: LSP process orphaned on Dial failure.If
jsonrpc2.Dialfails (line 500), the spawned LSP process and its pipes remain open. WhilecancelFunc()is called (line 504), the child process itself is not terminated and the stdin/stdout pipes are not explicitly closed, leading to resource leaks.Ensure cleanup by adding:
dialer := base.NewStdDialer(stdin, stdout) rpc, err := jsonrpc2.Dial(ctx, dialer, jsonrpc2.ConnectionOptions{ Handler: base.NewChainHandler(base.LogHandler(log)), }) if err != nil { cancelFunc() + stdin.Close() + stdout.Close() + if cmd.Process != nil { + _ = cmd.Process.Kill() + } log.Error(err, "unable to connect over new package") return nil, additionalBuiltinConfig, err }lsp/base_service_client/base_service_client.go (1)
240-240: Use InitializedParams for "initialized" notification.The LSP "initialized" notification should send
protocol.InitializedParams{}, notprotocol.InitializeParams{}.Apply this diff:
-err = sc.Conn.Notify(sc.Ctx, "initialized", protocol.InitializeParams{}) +err = sc.Conn.Notify(sc.Ctx, "initialized", protocol.InitializedParams{})lsp/base_service_client/std_dialer.go (1)
19-19: Fix documentation: CmdDialer → StdDialer.The comment references "CmdDialer" but the actual type is "StdDialer".
Apply this diff:
-// Create a new CmdDialer +// NewStdDialer creates a new StdDialer wrapping the provided stdin/stdout streams. func NewStdDialer(stdin io.WriteCloser, stdout io.ReadCloser) jsonrpc2.Dialer {provider/grpc/provider.go (2)
288-291: Replace debug print with structured log; always tail stdout.Apply:
- fmt.Printf("\ncommand: %v\n", cmd) - if out != nil { - go LogProviderOut(context.Background(), out, log) - } + log.V(3).Info("starting provider binary", "command", cmd.String()) + go LogProviderOut(context.Background(), out, log)
298-303: Critical: return error on failed dial.Currently logs and returns
conn(possibly nil), leading to panics downstream.Apply:
- conn, err := socket.ConnectGRPC(connectionString) - - if err != nil { - log.Error(err, "did not connect") - } - return conn, out, nil + conn, err := socket.ConnectGRPC(connectionString) + if err != nil { + log.Error(err, "did not connect") + return nil, nil, err + } + return conn, out, nilprovider/grpc/socket/pipe_windows.go (2)
54-61: Remove debug print and validate target prefix.Apply:
- pipeName, _ := strings.CutPrefix(connectionString, "unix://") + pipeName, ok := strings.CutPrefix(connectionString, "unix://") + if !ok || pipeName == "" { + return nil, fmt.Errorf("unexpected windows pipe target: %q; want unix://<pipe>", connectionString) + } pipe, err := winio.DialPipeContext(ctx, pipeName) if err != nil { return nil, err } - fmt.Printf("pipe: %#v", pipe) return pipe, nil
19-24: Unseeded math/rand can cause repeat pipe names across runs.Seed once to avoid deterministic collisions.
Apply:
import ( "context" "fmt" "io" "math/rand" "net" "strings" + "time" "github.com/Microsoft/go-winio" // ... ) +func init() { + rand.Seed(time.Now().UnixNano()) +}
🧹 Nitpick comments (6)
provider/server.go (4)
103-111: UDS startup: best‑effort cleanup of stale socket.Before listening on a Unix socket, unlink any stale path to avoid EADDRINUSE on restart.
Apply:
} else if s.SocketPath != "" { - listen, err = socket.Listen(s.SocketPath) + _ = os.Remove(s.SocketPath) // ignore error if not present + listen, err = socket.Listen(s.SocketPath)
121-131: Add max message size limits to TLS server too.Parity with the non‑TLS branch prevents large messages from being rejected only in TLS.
Apply:
- gs = grpc.NewServer(grpc.Creds(creds), grpc.UnaryInterceptor(s.authUnaryInterceptor)) + gs = grpc.NewServer( + grpc.Creds(creds), + grpc.UnaryInterceptor(s.authUnaryInterceptor), + grpc.MaxRecvMsgSize(socket.MAX_MESSAGE_SIZE), + grpc.MaxSendMsgSize(socket.MAX_MESSAGE_SIZE), + )- gs = grpc.NewServer(grpc.Creds(creds)) + gs = grpc.NewServer( + grpc.Creds(creds), + grpc.MaxRecvMsgSize(socket.MAX_MESSAGE_SIZE), + grpc.MaxSendMsgSize(socket.MAX_MESSAGE_SIZE), + )
272-276: Use the server’s RNG instance for ID generation.Stay consistent with seeded
s.randinstead of globalrand.Apply:
- id := rand.Int63() + id := s.rand.Int63()
282-287: Stored context likely canceled; prefer long‑lived ctx or drop field.You store the request ctx; if unused or for lifecycle, store
newCtxinstead or remove.Apply:
- ctx: ctx, + ctx: newCtx,provider/grpc/provider.go (2)
132-148: Deterministic service wait with ticker + timeout.Avoid select default randomness; use a ticker and a fixed timeout.
Example:
deadline := time.After(30 * time.Second) tick := time.NewTicker(3 * time.Second) defer tick.Stop() for { select { case <-tick.C: services, err := refClt.ListServices() if err == nil && len(services) != 0 { return services, nil } if err != nil { log.Error(err, "error for list services retrying") } case <-deadline: return nil, fmt.Errorf("no services found") } }
48-76: Avoid storing a context that’s canceled before return.
refCltCtxis deferred-canceled; don’t assign it intogp.ctx. UsectxCmdor a fresh background.Apply:
- gp := grpcProvider{ + gp := grpcProvider{ // ... - ctx: refCltCtx, + ctx: ctxCmd,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumprovider/internal/grpc/library.pb.gois excluded by!**/*.pb.goprovider/internal/grpc/library_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
Dockerfile(1 hunks)Dockerfile.windows(0 hunks)Makefile(2 hunks)debug/debug.Dockerfile(0 hunks)demo.Dockerfile(1 hunks)external-providers/dotnet-external-provider/main.go(3 hunks)external-providers/generic-external-provider/Dockerfile(2 hunks)external-providers/generic-external-provider/entrypoint.sh(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/generic-external-provider/main.go(3 hunks)external-providers/generic-external-provider/pkg/generic_external_provider/provider.go(1 hunks)external-providers/java-external-provider/go.mod(1 hunks)external-providers/java-external-provider/main.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go(0 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(6 hunks)external-providers/yq-external-provider/go.mod(2 hunks)external-providers/yq-external-provider/main.go(3 hunks)external-providers/yq-external-provider/pkg/yq_provider/service_client.go(0 hunks)go.mod(4 hunks)jsonrpc2/backoff_handler.go(0 hunks)jsonrpc2/handler.go(0 hunks)jsonrpc2/jsonrpc2.go(0 hunks)jsonrpc2/rpcerr.go(0 hunks)jsonrpc2/stream.go(0 hunks)jsonrpc2/wire.go(0 hunks)lsp/base_service_client/base_service_client.go(4 hunks)lsp/base_service_client/std_dialer.go(1 hunks)provider/grpc/provider.go(6 hunks)provider/grpc/service_client.go(1 hunks)provider/grpc/socket/consts.go(1 hunks)provider/grpc/socket/pipe_windows.go(1 hunks)provider/grpc/socket/uds.go(1 hunks)provider/internal/grpc/library.proto(2 hunks)provider/provider.go(3 hunks)provider/server.go(9 hunks)provider_container_settings.json(3 hunks)provider_pod_local_settings.json(1 hunks)
💤 Files with no reviewable changes (10)
- Dockerfile.windows
- external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go
- external-providers/yq-external-provider/pkg/yq_provider/service_client.go
- debug/debug.Dockerfile
- jsonrpc2/backoff_handler.go
- jsonrpc2/handler.go
- jsonrpc2/jsonrpc2.go
- jsonrpc2/rpcerr.go
- jsonrpc2/stream.go
- jsonrpc2/wire.go
🚧 Files skipped from review as they are similar to previous changes (12)
- external-providers/generic-external-provider/main.go
- provider/provider.go
- Dockerfile
- external-providers/yq-external-provider/main.go
- provider/grpc/service_client.go
- go.mod
- provider/grpc/socket/uds.go
- external-providers/yq-external-provider/go.mod
- Makefile
- provider_pod_local_settings.json
- external-providers/generic-external-provider/Dockerfile
- demo.Dockerfile
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-21T16:25:40.903Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: lsp/base_service_client/base_service_client.go:212-214
Timestamp: 2025-10-21T16:25:40.903Z
Learning: In `lsp/base_service_client/base_service_client.go`, when an RPC connection is provided via `c.RPC` (not nil), it is treated as already initialized and no handler setup is performed. This is known technical debt that needs to be addressed in the future to ensure LSP notifications like `textDocument/publishDiagnostics` are properly processed.
Applied to files:
lsp/base_service_client/base_service_client.goprovider/server.go
📚 Learning: 2025-07-25T02:05:02.884Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The `go` directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate `toolchain` directive (introduced in Go 1.21), e.g., `toolchain go1.23.11`.
Applied to files:
external-providers/java-external-provider/go.mod
📚 Learning: 2025-07-28T15:28:31.377Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:22-26
Timestamp: 2025-07-28T15:28:31.377Z
Learning: For Windows named pipe address generation in the analyzer-lsp project, cryptographic security is not required. Basic randomness using math/rand is sufficient for collision avoidance in local inter-process communication scenarios.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-07-28T15:27:30.233Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/grpc/socket/pipe_windows.go:28-37
Timestamp: 2025-07-28T15:27:30.233Z
Learning: In Windows named pipe gRPC connections, the "unix://" URI scheme prefix must be used instead of a more semantically correct scheme like "pipe://" because it's required for the gRPC package to provide the desired behavior. This is an intentional design choice despite being semantically incorrect.
Applied to files:
provider/grpc/socket/pipe_windows.go
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
PR: konveyor/analyzer-lsp#860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
provider/server.go
🧬 Code graph analysis (9)
external-providers/dotnet-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
lsp/base_service_client/std_dialer.go (1)
jsonrpc2_v2/serve.go (2)
Dialer(34-37)Dial(57-64)
lsp/base_service_client/base_service_client.go (6)
provider/provider.go (1)
RPCClient(161-165)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)lsp/base_service_client/base_handlers.go (1)
NewChainHandler(33-45)jsonrpc2_v2/serve.go (2)
Dialer(34-37)Dial(57-64)lsp/base_service_client/cmd_dialer.go (1)
NewCmdDialer(25-55)jsonrpc2_v2/conn.go (1)
ConnectionOptions(46-60)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (5)
lsp/base_service_client/std_dialer.go (1)
NewStdDialer(20-27)jsonrpc2_v2/serve.go (1)
Dial(57-64)jsonrpc2_v2/conn.go (1)
ConnectionOptions(46-60)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)lsp/base_service_client/base_handlers.go (2)
NewChainHandler(33-45)LogHandler(19-24)
provider/grpc/provider.go (5)
provider/provider.go (1)
Config(88-98)provider/internal/grpc/library.pb.go (3)
Config(78-89)Config(102-102)Config(117-119)provider/grpc/socket/pipe_windows.go (3)
GetAddress(19-24)GetConnectionString(26-28)ConnectGRPC(30-48)provider/grpc/socket/uds.go (3)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)
external-providers/java-external-provider/main.go (1)
provider/server.go (1)
NewServer(65-100)
external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
provider/provider.go (1)
BaseClient(450-454)
provider/grpc/socket/pipe_windows.go (4)
provider/grpc/socket/uds.go (5)
GetAddress(16-25)GetConnectionString(27-29)ConnectGRPC(35-42)Listen(31-33)ConnectRPC(44-55)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/serve.go (2)
Listener(18-31)Dial(57-64)jsonrpc2_v2/conn.go (2)
Connection(66-79)ConnectionOptions(46-60)
provider/server.go (5)
provider/provider.go (2)
BaseClient(450-454)Client(445-448)provider/grpc/socket/pipe_windows.go (2)
Listen(50-52)ConnectRPC(64-75)provider/grpc/socket/uds.go (2)
Listen(31-33)ConnectRPC(44-55)provider/grpc/socket/consts.go (1)
MAX_MESSAGE_SIZE(4-4)jsonrpc2_v2/jsonrpc2.go (1)
Handler(60-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (windows-latest)
🔇 Additional comments (20)
external-providers/generic-external-provider/pkg/generic_external_provider/provider.go (1)
33-33: LGTM! Returning interface type improves flexibility.Changing the return type from the concrete
*genericProviderto theprovider.BaseClientinterface follows Go best practices and improves testability and flexibility.external-providers/java-external-provider/pkg/java_external_provider/service_client.go (5)
24-24: LGTM! Import updated for jsonrpc2_v2 migration.The import path has been correctly updated to use the new
jsonrpc2_v2package while maintaining thejsonrpc2alias for compatibility with existing code.
72-74: LGTM! Filepath handling is safe.The length check on line 72 ensures the slice has at least one element before accessing index 0 on line 73. In Go,
len()safely returns 0 for nil slices, making this guard sufficient.
254-254: LGTM! Await-based RPC call is correctly implemented.The migration to the Await pattern is correct, with proper context usage and error handling that checks for closed RPC connections.
312-312: LGTM! Shutdown RPC call properly handles context lifecycle.The Await-based call correctly uses the shutdown context that is properly canceled via the defer on line 295.
411-420: LGTM! Initialization retry logic correctly migrated to Await pattern.The RPC call properly uses the Await pattern with appropriate error handling and retry logic. The error variable is correctly scoped within the loop, and the break on line 420 ensures the loop exits on successful initialization.
external-providers/generic-external-provider/go.mod (3)
32-37: No compatibility issues found — versions are consistent and correctly resolved.The transitive dependencies in
external-providers/generic-external-provider/go.modare identical to those in the rootgo.mod:
google.golang.org/grpc v1.73.0andgoogle.golang.org/protobuf v1.36.6match the root module's direct requiresgolang.org/x/net v0.38.0,golang.org/x/sys v0.31.0, andgolang.org/x/text v0.23.0match the root module's transitive dependenciesgoogle.golang.org/genproto/googleapis/rpc v0.0.0-20250324211829-b45e905df463matches the root module's indirect dependencyThe external provider correctly marks these as
// indirectbecause they are inherited transitively via thereplace github.com/konveyor/analyzer-lsp => ../../directive. This is the expected behavior and confirms proper dependency resolution.
16-16: go-winio v0.6.2 is verified as stable and secure—no action required.Version v0.6.2 is published and actively used by projects like OpenTelemetry, and no security advisories or CVEs were found in GitHub Advisory Database or NVD. The dependency aligns with the PR objective of adding Windows named pipe support and is safe to merge.
27-27: No changes required — OpenTelemetry v1.35.0 is backward-compatible.The v1.35.0 release (published 2025-03-05) is a backward-compatible minor update, with key changes including logging additions, semantic conventions upgrades, and bug fixes. This is the last release supporting Go 1.22; the next release requires Go 1.23—your codebase targets Go 1.23.9, so this is not a concern.
The async RPC migration uses
jsonrpc2_v2directly; OpenTelemetry is an indirect dependency. No breaking changes affect this pattern.external-providers/java-external-provider/go.mod (1)
6-47: LGTM: Routine dependency updates.The dependency version bumps are consistent with the broader PR's upgrade trajectory (OpenTelemetry, gRPC, protobuf ecosystem updates). All changes are minor/patch version increases with no breaking changes expected.
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
477-477: Good addition for diagnostics.The error log provides useful context for language server startup failures, aiding troubleshooting during retries.
lsp/base_service_client/std_dialer.go (1)
11-49: Well-structured dialer implementation.The StdDialer correctly implements the jsonrpc2.Dialer interface with clean delegation to underlying stdin/stdout streams. The Close method properly combines errors from both streams.
lsp/base_service_client/base_service_client.go (1)
306-312: Good handler chaining pattern.The handler delegation approach correctly tries the custom handler first and falls through to the base handler on unhandled methods. This provides extensibility while maintaining default behavior.
provider/grpc/socket/consts.go (1)
1-5: LGTM: Reasonable message size constant.8MB is a sensible default for gRPC message size limits in socket-based transports. The constant is appropriately scoped and well-named.
provider/internal/grpc/library.proto (1)
23-24: LGTM: Well-defined proto fields for socket support.The new
languageServerPipeandinitializedfields support the socket-based serving pattern. Field names are correctly spelled, and field numbers (7, 8) follow sequential ordering without conflicts.external-providers/dotnet-external-provider/main.go (1)
21-21: LGTM: Consistent socket support pattern.The socket flag, validation logic, and server initialization follow the same pattern as other external providers in this PR. The validation correctly requires either a port or socket, and error messaging is clear.
Also applies to: 38-40, 59-59
external-providers/java-external-provider/main.go (1)
17-17: LGTM: Socket support properly integrated.The socket flag and validation logic mirror the pattern established in other providers. The implementation is consistent and correct.
Also applies to: 41-43, 61-61
provider/server.go (1)
253-266: LGTM: RPC client uses background ctx and handler.Correct lifecycle choice; connection won’t be tied to Init request.
Based on learnings.
provider/grpc/provider.go (1)
194-207: LGTM: config propagation to pb.Config (incl. pipe/init flags).provider/grpc/socket/pipe_windows.go (1)
26-39: LGTM: use unix:// with passthrough for named pipes (intentional quirk).Based on learnings.
|
@shawn-hurley I just have a question about Kai, are you looking to make a change in the RPC client passed by the kai_analyzer_rpc, or it can work for now with the old client? |
|
@shawn-hurley the wrapper changes look good to me. do we have to update the documentation anywhere with the support for socket mode? I see the sample config files. |
|
@savitharaghunathan +1 on documentation update (just in general lol). I am going to create a follow-up issue to track that if that is ok? @pranavgaikwad Depending on how today goes, and how far @djzager is on breaking out the new extension, there should be changes going into kai_analyzer_rpc. What are you looking to do in that? |
Right now I have two extensions split but the java extension doesn't really do anything. I'm trying to see how far I can go without needing java-external-provider being pulled out of kai-analyzer-rpc. Should be ready for POC kai-analyzer-rpc and java-external-provider early next week. |
5 participants
Summary by CodeRabbit
New Features
Chores