Skip to content

Commit 1ea406f

Browse files
anton-107Isaac
andcommitted
[SSH] Read the serverless port forwarding setting once, without retries
The setting check used the default SDK client, which retries 429 and 503 until its retry budget runs out, so a slow settings service could hold up a working connection for minutes. It now reads through the no-retry probe client the AI Gateway check already uses. The check is also skipped when reconnecting with --metadata. The ProxyCommand child runs Run again with the server already running, so the read prevented nothing and was repeated on every IDE reconnect. Co-authored-by: Isaac <no-reply@databricks.com>
1 parent a441f86 commit 1ea406f

3 files changed

Lines changed: 49 additions & 20 deletions

File tree

‎experimental/ssh/internal/client/agentshim.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ func probeAIGateway(ctx context.Context, wsclient *databricks.WorkspaceClient) e
360360
}
361361

362362
// newProbeAPIClient builds a low-level client from cfg with the SDK's retries
363-
// turned off, so the gateway preflight is one-shot. Clearing ErrorRetriable and
363+
// turned off, so a preflight check is one-shot. Clearing ErrorRetriable and
364364
// TransientErrors makes a retriable status (429, 503, ...) surface on the first
365365
// attempt rather than looping until the retry budget expires; it mirrors
366366
// client.New, which builds the retrying client the rest of the CLI uses.

‎experimental/ssh/internal/client/client.go‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,9 @@ func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOpt
355355
cmdio.LogString(ctx, fmt.Sprintf("Connecting to %s...", sessionID))
356356
}
357357

358-
if opts.IsServerlessMode() {
358+
// Skipped when reconnecting with --metadata (the ProxyCommand child): the server is already
359+
// running, so there is no compute left to spare, and every IDE reconnect would repeat the read.
360+
if opts.IsServerlessMode() && opts.ServerMetadata == "" {
359361
if err := checkPortForwardingToServerless(ctx, client); err != nil {
360362
return err
361363
}
@@ -710,9 +712,16 @@ func metadataResponseError(statusCode int, body []byte) error {
710712
// (PortForwardingToServerlessGate in data-plane-router) also depends on an enforcement flag and a port
711713
// allow-list that the CLI cannot see, and the setting is not readable through the public API in every
712714
// environment. A read that fails therefore lets the connection go ahead, and the proxy's own 403
713-
// (errDriverProxyForbidden) remains the backstop.
715+
// (errDriverProxyForbidden) remains the backstop. The read goes through a client with retries turned
716+
// off, so a throttled or unavailable settings service cannot hold up the connection for the SDK's
717+
// retry budget.
714718
func checkPortForwardingToServerless(ctx context.Context, client *databricks.WorkspaceClient) error {
715-
setting, err := client.WorkspaceSettingsV2.GetPublicWorkspaceSetting(ctx, settingsv2.GetPublicWorkspaceSettingRequest{
719+
apiClient, err := newProbeAPIClient(client.Config)
720+
if err != nil {
721+
log.Debugf(ctx, "Could not build a client to read the %s setting, continuing: %v", portFwdServerlessSetting, err)
722+
return nil
723+
}
724+
setting, err := settingsv2.NewWorkspaceSettingsV2(apiClient).GetPublicWorkspaceSetting(ctx, settingsv2.GetPublicWorkspaceSettingRequest{
716725
Name: portFwdServerlessSetting,
717726
})
718727
if err != nil {

‎experimental/ssh/internal/client/client_internal_test.go‎

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@ import (
55
"encoding/json"
66
"errors"
77
"fmt"
8+
"net/http"
9+
"net/http/httptest"
810
"slices"
911
"strconv"
1012
"strings"
13+
"sync/atomic"
1114
"testing"
1215
"time"
1316

@@ -20,7 +23,6 @@ import (
2023
"github.com/databricks/databricks-sdk-go/service/compute"
2124
"github.com/databricks/databricks-sdk-go/service/environments"
2225
"github.com/databricks/databricks-sdk-go/service/jobs"
23-
"github.com/databricks/databricks-sdk-go/service/settingsv2"
2426
"github.com/stretchr/testify/assert"
2527
"github.com/stretchr/testify/mock"
2628
"github.com/stretchr/testify/require"
@@ -863,38 +865,56 @@ func TestMetadataResponseError(t *testing.T) {
863865
func TestCheckPortForwardingToServerless(t *testing.T) {
864866
tests := []struct {
865867
name string
866-
setting *settingsv2.Setting
867-
readErr error
868+
status int
869+
body string
868870
wantErr error
869871
}{
870872
{
871873
name: "disabled",
872-
setting: &settingsv2.Setting{EffectiveBooleanVal: &settingsv2.BooleanMessage{Value: false}},
874+
status: http.StatusOK,
875+
body: `{"effective_boolean_val": {"value": false}}`,
873876
wantErr: errPortForwardingDisabled,
874877
},
875878
{
876-
name: "enabled",
877-
setting: &settingsv2.Setting{EffectiveBooleanVal: &settingsv2.BooleanMessage{Value: true}},
879+
name: "enabled",
880+
status: http.StatusOK,
881+
body: `{"effective_boolean_val": {"value": true}}`,
878882
},
879883
{
880-
name: "no effective value",
881-
setting: &settingsv2.Setting{},
884+
name: "no effective value",
885+
status: http.StatusOK,
886+
body: `{}`,
882887
},
883888
{
884889
// Production does not serve the setting through the public API yet.
885-
name: "read fails",
886-
readErr: errors.New("Setting port_fwd_serverless_enabled is not enabled for public API"),
890+
name: "read fails",
891+
status: http.StatusBadRequest,
892+
body: `{"error_code": "INVALID_PARAMETER_VALUE", "message": "Setting port_fwd_serverless_enabled is not enabled for public API"}`,
893+
},
894+
{
895+
// A retriable status is not retried, so a throttled service cannot hold up the connection.
896+
name: "throttled",
897+
status: http.StatusTooManyRequests,
898+
body: `{"error_code": "TOO_MANY_REQUESTS", "message": "slow down"}`,
887899
},
888900
}
889901
for _, tc := range tests {
890902
t.Run(tc.name, func(t *testing.T) {
891-
ctx := cmdio.MockDiscard(t.Context())
892-
m := mocks.NewMockWorkspaceClient(t)
893-
m.GetMockWorkspaceSettingsV2API().EXPECT().
894-
GetPublicWorkspaceSetting(mock.Anything, settingsv2.GetPublicWorkspaceSettingRequest{Name: portFwdServerlessSetting}).
895-
Return(tc.setting, tc.readErr)
903+
var reads atomic.Int32
904+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
905+
if r.URL.Path != "/api/2.1/settings/"+portFwdServerlessSetting {
906+
w.WriteHeader(http.StatusNotFound)
907+
return
908+
}
909+
reads.Add(1)
910+
w.WriteHeader(tc.status)
911+
_, _ = w.Write([]byte(tc.body))
912+
}))
913+
defer srv.Close()
896914

897-
assert.Equal(t, tc.wantErr, checkPortForwardingToServerless(ctx, m.WorkspaceClient))
915+
ctx := cmdio.MockDiscard(t.Context())
916+
assert.Equal(t, tc.wantErr, checkPortForwardingToServerless(ctx, newProbeClient(t, srv.URL)))
917+
assert.Equal(t, int32(1), reads.Load())
898918
})
899919
}
900920
}

0 commit comments

Comments
 (0)