Skip to content

Fix Gravity metadata nil session client panic#289

Merged
jhaynie merged 1 commit into
mainfrom
fix/gravity-nil-session-client
Jun 4, 2026
Merged

Fix Gravity metadata nil session client panic#289
jhaynie merged 1 commit into
mainfrom
fix/gravity-nil-session-client

Conversation

@jhaynie
Copy link
Copy Markdown
Member

@jhaynie jhaynie commented Jun 4, 2026

Summary

  • select a non-nil Gravity session client for metadata calls instead of directly using sessionClients[0]
  • return a normal error when no usable session client exists
  • add regression coverage for empty, all-nil, and nil-first session client slices
  • update go.mod to Go 1.26.4

Test

  • go test ./gravity

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced resilience for deployment and sandbox metadata retrieval to properly handle situations where session clients are unavailable, automatically routing requests to available alternatives.
  • Tests

    • Added comprehensive test coverage for deployment metadata retrieval, including proper error handling when no session clients are available and automatic fallback routing behavior.
  • Chores

    • Updated Go toolchain version requirement.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The PR improves resilience of metadata RPC calls by introducing a helper function to select the first non-nil session client and refactoring both GetDeploymentMetadata and GetSandboxMetadata to use it. Test infrastructure is enhanced to support configurable mock behavior, and new tests validate the error and client selection paths. The Go toolchain is also bumped to version 1.26.4.

Changes

Session client resilience for metadata RPCs

Layer / File(s) Summary
Session client selection and RPC refactoring
gravity/grpc_client.go
Added usableSessionClient() helper to locate the first non-nil entry in g.sessionClients. Refactored GetDeploymentMetadata and GetSandboxMetadata to call the helper instead of assuming sessionClients[0] exists, and updated RPC invocations to use the selected client.
Test infrastructure and coverage
gravity/control_stream_resilience_test.go
Updated mockSessionClient with configurable deployment and sandbox metadata responses, errors, and call counters. Added three new test cases to validate GetDeploymentMetadata error handling when session clients are absent or all nil, and correct routing to the first usable non-nil client.
Go toolchain version update
go.mod
Go toolchain directive bumped from go 1.26.2 to go 1.26.4.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
gravity/control_stream_resilience_test.go (1)

278-320: ⚡ Quick win

Add mirrored regression tests for GetSandboxMetadata.

Line 278+ adds strong coverage for GetDeploymentMetadata, but the same nil-client selection path in GetSandboxMetadata remains untested. Please add equivalent cases for empty clients, all-nil clients, and later non-nil client selection.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gravity/control_stream_resilience_test.go` around lines 278 - 320, Add three
mirrored tests for GetSandboxMetadata:
TestGetSandboxMetadata_NoSessionClientsReturnsError,
TestGetSandboxMetadata_AllNilSessionClientsReturnsError, and
TestGetSandboxMetadata_UsesLaterNonNilSessionClient. For the first, create g :=
newResilienceTestClient(t, 0, false) and call g.GetSandboxMetadata(...), assert
an error contains "no gRPC session clients available". For the second, create g
:= newResilienceTestClient(t, 2, true), set g.sessionClients =
[]pb.GravitySessionServiceClient{nil, nil}, call GetSandboxMetadata and assert
an error contains "no usable gRPC session clients available". For the third,
create g := newResilienceTestClient(t, 3, true), set a laterClient :=
&mockSessionClient{sandboxResp: &pb.SandboxMetadataResponse{Success: true}} and
g.sessionClients = []pb.GravitySessionServiceClient{nil, laterClient, nil}, call
GetSandboxMetadata, assert no error, resp.GetSuccess() is true, and
laterClient.sandboxCalls == 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@gravity/control_stream_resilience_test.go`:
- Around line 278-320: Add three mirrored tests for GetSandboxMetadata:
TestGetSandboxMetadata_NoSessionClientsReturnsError,
TestGetSandboxMetadata_AllNilSessionClientsReturnsError, and
TestGetSandboxMetadata_UsesLaterNonNilSessionClient. For the first, create g :=
newResilienceTestClient(t, 0, false) and call g.GetSandboxMetadata(...), assert
an error contains "no gRPC session clients available". For the second, create g
:= newResilienceTestClient(t, 2, true), set g.sessionClients =
[]pb.GravitySessionServiceClient{nil, nil}, call GetSandboxMetadata and assert
an error contains "no usable gRPC session clients available". For the third,
create g := newResilienceTestClient(t, 3, true), set a laterClient :=
&mockSessionClient{sandboxResp: &pb.SandboxMetadataResponse{Success: true}} and
g.sessionClients = []pb.GravitySessionServiceClient{nil, laterClient, nil}, call
GetSandboxMetadata, assert no error, resp.GetSuccess() is true, and
laterClient.sandboxCalls == 1.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 75c9e901-ac56-470c-be45-b8d59f461d40

📥 Commits

Reviewing files that changed from the base of the PR and between a95898d and 54130d7.

📒 Files selected for processing (3)
  • go.mod
  • gravity/control_stream_resilience_test.go
  • gravity/grpc_client.go
📜 Review details
⏰ 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: build
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
gravity/grpc_client.go (1)

7047-7060: LGTM!

Also applies to: 7062-7077, 7082-7096

gravity/control_stream_resilience_test.go (1)

20-23: LGTM!

Also applies to: 27-28, 48-54, 59-65

go.mod (1)

3-3: Update go.mod to go 1.26.4: valid stable release with security fixes

go 1.26.4 is an official stable Go release (June 2, 2026) and is the latest patch version for the 1.26 line; it includes security fixes (including issues in crypto/x509, mime, and net/textproto).

@jhaynie jhaynie merged commit 7db4fd7 into main Jun 4, 2026
5 checks passed
@jhaynie jhaynie deleted the fix/gravity-nil-session-client branch June 4, 2026 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant