Repository navigation
Conversation
The Go SDK now does what the CLI already does for long-lived sandbox calls. The GetSandbox check that Exec.Run, Exec.Stream, Exec.Interactive, TCP.Forward and SSH.Tunnel already make also asks the gateway for the owner replica. The call then sends it in the x-openshell-replica header and retries once without the header when the edge proxy reports that the owner's route has no ready endpoints. No extra round trip is added. Without a reported owner the calls are unchanged. Part of NVIDIA#4223 Signed-off-by: divesh <dgude@nvidia.com>
EmilienM
left a comment
There was a problem hiding this comment.
Needs a human read: 8 minor (inline, 1 below)
No blocking or major findings: this is a faithful port of the CLI's shipped design, and the grpc-go Header()/RecvMsg(nil) handling checks out against v1.83.2. Deep-tier review on code changes, so the verdict stays with a human.
Outside the diff:
- Minor:
docs/kubernetes/ingress.mdx:260, the docs still say only the CLI sets the replica header; a one-sentence update should include the Go SDK.
How this was reviewed
- Commit
32b34f8f, round 1 - Deep review, because of sdk/go/openshell/v1/routing.go, exec_client.go, tcp_client.go
- Passes: architecture, performance, security, scalability, then correctness
- Independent skeptics checked each candidate finding (2 for the major, 1 for minor ones); 5 of 7 held up
- Repo rules read from
9a85667d: AGENTS.md, CLAUDE.md, CONTRIBUTING.md, docs/CONTRIBUTING.mdx - Not covered: nothing skipped
| // Metadata that lets an edge proxy send a sandbox's long-lived calls straight | ||
| // to the gateway replica holding its supervisor session. | ||
| const ( | ||
| ownerReplicaHeader = "x-openshell-owner" |
There was a problem hiding this comment.
Minor: The header names and the retryUnrouted substrings are hand-copied from crates/openshell-core/src/replica_routing.rs and the CLI's retry_unrouted, and nothing points at a shared spec, so the two languages can drift silently: a header rename just stops routing, an Envoy wording change breaks the fallback. One paragraph in docs/kubernetes/ingress.mdx (header trio plus the fallback rule), referenced from both sides, would pin the contract.
| // sandboxOwner checks that the sandbox exists and returns the replica that | ||
| // owns its supervisor session, or "" when the gateway reports none. | ||
| func sandboxOwner(ctx context.Context, sandboxes SandboxInterface, workspace, name string) (string, error) { | ||
| if g, ok := sandboxes.(ownerGetter); ok { |
There was a problem hiding this comment.
Minor: The capability is found by asserting SandboxInterface down to ownerGetter, so a future wrapper (caching, retry) silently loses owner routing and falls back to the relay hop. Taking *sandboxClient, or an internal interface with getWithOwner, in the package-private constructors makes it a compile-time property.
| // Header blocks until the proxy answers; nil metadata means the stream | ||
| // ended without headers, and RecvMsg then returns its status. | ||
| var md metadata.MD | ||
| if md, err = stream.Header(); err == nil && md == nil { |
There was a problem hiding this comment.
Minor: With an owner present, Header() blocks until the gateway handler returns, so Interactive/Forward/Tunnel setup now waits on sandbox resolution and relay setup (up to the ~15s session-wait during a supervisor reconnect) where it used to return immediately. That matches the CLI's semantics; a line in the Go SDK docs would help. Also tcp_client.go:65, ssh_client.go:103.
| } | ||
| } | ||
| if retryUnrouted(err) { | ||
| return open(ctx) |
There was a problem hiding this comment.
Minor: The retry skips the Header() check the first attempt gets, so a second unroutable failure only shows up on the caller's first Recv. The CLI awaits the retry's response and fails at the call site (run.rs:2112).
|
|
||
| streamCtx, cancel := context.WithCancel(ctx) | ||
| stream, err := e.client.ExecSandboxInteractive(streamCtx) | ||
| stream, err := openRouted(streamCtx, owner, func(ctx context.Context) (grpc.BidiStreamingClient[pb.ExecSandboxInput, pb.ExecSandboxEvent], error) { |
There was a problem hiding this comment.
Minor: This closure repeats openForward's open-bidi / send-init / EOF-tolerance sequence (same comment at routing.go:72). A generic openBidiRouted[I, O] would keep the EOF rule in one place.
| } | ||
| // io.EOF means the stream already ended; its status comes from Recv. | ||
| if err := s.Send(&pb.ExecSandboxInput{Payload: &pb.ExecSandboxInput_Start{Start: startReq}}); err != nil && err != io.EOF { | ||
| return s, err |
There was a problem hiding this comment.
Minor: On the no-owner path nothing checks the stream before returning, so a start-frame io.EOF now surfaces as a bare io.EOF on first use instead of failing the call; the routed path catches it via Header(). Same for openForward (routing.go:72).
| } | ||
|
|
||
| // getWithOwner is Get that also asks the gateway for the owner replica. | ||
| func (s *sandboxClient) getWithOwner(ctx context.Context, workspace, name string) (*Sandbox, string, error) { |
There was a problem hiding this comment.
Minor: getWithOwner copies Get's request construction and conversion. A shared private get(ctx, workspace, name, opts ...grpc.CallOption) keeps them from drifting.
Summary
The Go SDK now sends exec, port-forward, and SSH tunnel calls straight to the gateway replica that owns the sandbox's supervisor session, as the CLI already does. Behind an edge proxy with replica routing enabled, this skips the relay hop through a peer replica; without one, the header is ignored and behavior is unchanged.
Related Issue
Part of #4223 (the "Every SDK asks for the owner and sends the replica hint, like the CLI" item, Go portion). Rust is #4790; TypeScript and Python follow in separate PRs.
Changes
Exec.Run,Exec.Stream,Exec.Interactive,TCP.Forward, andSSH.Tunnelalready callGetSandboxfirst; that call now also asks for the owner, so no round trip is added.routing.go: sendsx-openshell-replicaon those streams and retries once without it when Envoy reports the owner's route has no ready endpoints (same rule as the CLI). Forward and interactive calls resend their init frame on retry.Release Notes
Go SDK exec, port-forward, and SSH tunnel calls now take the direct route to the sandbox's owner replica when replica routing is enabled.
Testing
mise run go:ci: format, build, lint, tests, proto and docs checks)Checklist