Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nextchanges/cli/auth-profiles-timeout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* `databricks auth profiles` no longer stalls on an unreachable workspace. Each profile is now validated with a 10s timeout (also applied to the host-metadata fetch in `EnsureResolved`), so a host the SDK would otherwise retry — connection refused, connect/TLS timeout, or a retriable 5xx — can't block the whole listing for the SDK's default ~5-minute retry budget.
25 changes: 23 additions & 2 deletions cmd/auth/profiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ import (
"gopkg.in/ini.v1"
)

// profileValidationTimeout bounds each per-profile validation so a host the SDK
// retries — connection refused, connect/TLS timeout, retriable 5xx — cannot
// stall the whole listing. Without it a single such host blocks `auth profiles`
// for the SDK's default retry budget (~5 minutes). Hosts that fail DNS are not
// retriable and already fail fast, so this only bounds the retriable cases.
// A var (not const) so tests can shrink it.
var profileValidationTimeout = 10 * time.Second

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why 10 seconds? Other CLI limits seem to be 30 seconds, maybe we should stick to this?


type profileMetadata struct {
Name string `json:"name"`
Host string `json:"host,omitempty"`
Expand All @@ -37,11 +45,21 @@ func (c *profileMetadata) IsEmpty() bool {
}

func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipValidate bool) {
timeoutSeconds := int(profileValidationTimeout / time.Second)
cfg := &config.Config{
Loaders: []config.Loader{config.ConfigFile},
ConfigFile: configFilePath,
Profile: c.Name,
DatabricksCliPath: env.Get(ctx, "DATABRICKS_CLI_PATH"),

// Bound the SDK's per-request and total-retry budgets to the same
// per-profile ceiling. EnsureResolved fetches host metadata via the
// SDK's retrier, which defaults to 5 minutes — and it runs on
// context.Background internally, so the context.WithTimeout below on the
// validation call cannot reach it. Without these a single unreachable
// host stalls the listing well past the validation timeout.
HTTPTimeoutSeconds: timeoutSeconds,
RetryTimeoutSeconds: timeoutSeconds,
}
if skipValidate {
// EnsureResolved fetches <host>/.well-known/databricks-config to enrich
Expand Down Expand Up @@ -72,13 +90,16 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV
log.Debugf(ctx, "Profile %q: overrode config type from %s to %s (SPOG host)", c.Name, cfg.ConfigType(), configType)
}

callCtx, cancel := context.WithTimeout(ctx, profileValidationTimeout)
defer cancel()

switch configType {
case config.AccountConfig:
a, err := databricks.NewAccountClient((*databricks.Config)(cfg))
if err != nil {
return
}
_, err = a.Workspaces.List(ctx)
_, err = a.Workspaces.List(callCtx)
c.Host = cfg.Host
c.AuthType = cfg.AuthType
if err != nil {
Expand All @@ -90,7 +111,7 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV
if err != nil {
return
}
_, err = w.CurrentUser.Me(ctx, iam.MeRequest{})
_, err = w.CurrentUser.Me(callCtx, iam.MeRequest{})
c.Host = cfg.Host
c.AuthType = cfg.AuthType
if err != nil {
Expand Down
47 changes: 47 additions & 0 deletions cmd/auth/profiles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"runtime"
"sync/atomic"
"testing"
"time"

"github.com/databricks/cli/libs/databrickscfg"
"github.com/databricks/databricks-sdk-go/config"
Expand Down Expand Up @@ -284,3 +285,49 @@ func TestProfileLoadNoDiscoveryStaysWorkspace(t *testing.T) {
assert.NotEmpty(t, p.Host)
assert.Equal(t, "pat", p.AuthType)
}

// TestProfileLoadTimesOutOnUnresponsiveHost is a regression test for a hang:
// the SDK retries transient network failures for ~5 minutes by default, so a
// host that accepts the connection but never responds would stall the whole
// `auth profiles` listing. profileValidationTimeout must bound the wait
// (including the EnsureResolved metadata fetch, which runs on context.Background
// and so is only reachable via HTTPTimeoutSeconds/RetryTimeoutSeconds).
//
// The timeout is shrunk to 2s here — kept >=1s because Load derives the SDK's
// integer-second budgets from it, and a sub-second value would floor to 0
// (which the SDK reads as "use the default", defeating the test).
func TestProfileLoadTimesOutOnUnresponsiveHost(t *testing.T) {
prev := profileValidationTimeout
profileValidationTimeout = 2 * time.Second
t.Cleanup(func() { profileValidationTimeout = prev })

// A server that hangs every request until the client gives up. Waiting on
// the request context (rather than a private channel) means the handler
// returns as soon as our timeout cancels the call, so server.Close doesn't
// block on a leaked connection during cleanup.
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
<-r.Context().Done()
}))
t.Cleanup(server.Close)

dir := t.TempDir()
configFile := filepath.Join(dir, ".databrickscfg")
t.Setenv("HOME", dir)
if runtime.GOOS == "windows" {
t.Setenv("USERPROFILE", dir)
}
content := "[hung]\nhost = " + server.URL + "\ntoken = test-token\n"
require.NoError(t, os.WriteFile(configFile, []byte(content), 0o600))

p := &profileMetadata{Name: "hung", Host: server.URL}
start := time.Now()
p.Load(t.Context(), configFile, false)
elapsed := time.Since(start)

assert.False(t, p.Valid, "an unresponsive host must not validate")
// Generously above the 2s bound (metadata fetch + validation call, each
// bounded) but far below the SDK's ~5m default that this timeout prevents.
// A regression that drops the bound would blow past this and the whole
// package would then hit the go test -timeout instead.
assert.Less(t, elapsed, 60*time.Second, "Load must be bounded by profileValidationTimeout, not the SDK default")
}
Loading