diff --git a/pkg/ide/opener/browser_tunnel.go b/pkg/ide/opener/browser_tunnel.go index 5418e28ba..0cfffd3bf 100644 --- a/pkg/ide/opener/browser_tunnel.go +++ b/pkg/ide/opener/browser_tunnel.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "net" + neturl "net/url" "os" "os/exec" "path/filepath" @@ -16,6 +18,15 @@ import ( "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/tunnel" "github.com/gofrs/flock" + "k8s.io/apimachinery/pkg/util/wait" +) + +// browserProbeBudget bounds how long openBrowserWhenReachable will wait for +// the target URL's port to accept TCP connections before giving up. +const ( + browserProbeBudget = 5 * time.Second + browserProbeInterval = 200 * time.Millisecond + browserProbeDial = 300 * time.Millisecond ) // KillBrowserTunnel terminates the detached browser tunnel for a workspace @@ -154,27 +165,35 @@ type browserIDEInvocation struct { // the tunnel inline because the daemon already runs out-of-process. // // If a tunnel is already running for this workspace, no new process is -// spawned; the existing tunnel is reused. +// spawned; the existing tunnel is reused and its recorded TargetURL is +// returned (which may differ from tunnelParams.TargetURL when the caller +// allocated a new port that the existing helper isn't using). func startDetachedBrowserTunnel( ctx context.Context, params IDEParams, tunnelParams tunnel.BrowserTunnelParams, inv browserIDEInvocation, -) error { +) (string, error) { label := inv.Label openBrowser := inv.OpenBrowser - if _, ok := params.Client.(client.DaemonClient); ok { - return tunnel.StartBrowserTunnel(ctx, tunnelParams) - } - contextName := params.Client.Context() workspaceID := params.Client.Workspace() + if _, ok := params.Client.(client.DaemonClient); ok { + return runDaemonBrowserTunnel(ctx, daemonTunnelArgs{ + ContextName: contextName, + WorkspaceID: workspaceID, + TunnelParams: tunnelParams, + Inv: inv, + OpenBrowser: openBrowser, + }) + } + // Serialize concurrent attempts (e.g. parallel `devsy up`) to avoid // orphaning helper processes by racing on the state file. unlock, err := acquireTunnelLock(contextName, workspaceID) if err != nil { - return fmt.Errorf( + return "", fmt.Errorf( "acquire tunnel lock (refusing to spawn unlocked to avoid orphan helpers): %w", err, ) @@ -182,13 +201,13 @@ func startDetachedBrowserTunnel( defer unlock() // Reuse an existing live tunnel if present; clear stale state otherwise. - if tryReuseExistingTunnel(ctx, contextName, workspaceID, inv) { - return nil + if reusedURL, ok := tryReuseExistingTunnel(ctx, contextName, workspaceID, inv); ok { + return reusedURL, nil } pid, logLocation, err := spawnTunnelHelper(contextName, workspaceID, tunnelParams, label) if err != nil { - return err + return "", err } pkglog.Infof( @@ -201,7 +220,7 @@ func startDetachedBrowserTunnel( go openBrowserAsync(ctx, tunnelParams.TargetURL) } - return nil + return tunnelParams.TargetURL, nil } // spawnTunnelHelper locates the current executable, builds the helper @@ -400,18 +419,19 @@ func openBrowserAsync(ctx context.Context, url string) { } // tryReuseExistingTunnel reuses an already-running tunnel helper for the -// workspace, if any. It returns true if an existing live tunnel was found and -// reused (in which case the caller should not spawn a new helper). If state -// exists but the recorded PID is dead, the stale state file is removed and -// false is returned so the caller can proceed with a fresh spawn. +// workspace, if any. On reuse it returns the recorded TargetURL so the +// caller can surface the live URL (not its own freshly-computed one). +// If state exists but the recorded PID is dead, the stale state file is +// removed and ("", false) is returned so the caller can proceed with a +// fresh spawn. func tryReuseExistingTunnel( ctx context.Context, contextName, workspaceID string, inv browserIDEInvocation, -) bool { +) (string, bool) { existing, _ := ReadTunnelState(contextName, workspaceID) if existing == nil { - return false + return "", false } if helperMatchesState(existing) { pkglog.Infof( @@ -421,12 +441,147 @@ func tryReuseExistingTunnel( if inv.OpenBrowser { go openBrowserAsync(ctx, existing.TargetURL) } - return true + return existing.TargetURL, true } if statePath, err := TunnelStateFilePath(contextName, workspaceID); err == nil { _ = os.Remove(statePath) } - return false + return "", false +} + +// daemonTunnelArgs bundles the parameters for runDaemonBrowserTunnel to keep +// its signature small. +type daemonTunnelArgs struct { + ContextName string + WorkspaceID string + TunnelParams tunnel.BrowserTunnelParams + Inv browserIDEInvocation + OpenBrowser bool +} + +// runDaemonBrowserTunnel handles the daemon-client path: reuse an existing +// helper if one is live, otherwise start the in-process tunnel and open the +// browser only once the target URL becomes reachable. +func runDaemonBrowserTunnel(ctx context.Context, a daemonTunnelArgs) (string, error) { + unlock, err := acquireTunnelLock(a.ContextName, a.WorkspaceID) + if err != nil { + return "", fmt.Errorf( + "acquire tunnel lock (refusing to start unlocked to avoid orphan helpers): %w", + err, + ) + } + defer unlock() + + if reusedURL, ok := tryReuseExistingTunnel(ctx, a.ContextName, a.WorkspaceID, a.Inv); ok { + return reusedURL, nil + } + + // Derive a cancellable context for the probe goroutine. When + // StartBrowserTunnel returns (success or failure) the deferred + // cancelProbe stops the probe so it does not keep polling a port that + // will never come up and emit a misleading "didn't come up" warning + // alongside the original error. + probeCtx, cancelProbe := context.WithCancel(ctx) + defer cancelProbe() + + if a.OpenBrowser { + go openBrowserWhenReachable(probeCtx, a.TunnelParams.TargetURL) + } + return a.TunnelParams.TargetURL, tunnel.StartBrowserTunnel(ctx, a.TunnelParams) +} + +// openBrowserWhenReachable polls the target URL's TCP port until it accepts +// connections, then opens the browser. If ctx is cancelled before the port +// is reachable the goroutine exits silently — the caller already gave up +// (e.g. tunnel.StartBrowserTunnel failed) and warning about an unreachable +// URL would only duplicate that error. The "didn't come up" warning is +// reserved for the budget-expired case. +func openBrowserWhenReachable(ctx context.Context, url string) { + hostPort, err := hostPortFromURL(url) + if err != nil { + pkglog.Warnf("could not parse browser-tunnel URL %q: %v; skipping auto-open", url, err) + return + } + if err := probeTCPReachable(ctx, hostPort, browserProbeBudget); err != nil { + if errors.Is(err, context.Canceled) { + return + } + pkglog.Warnf( + "browser-tunnel URL %s never became reachable within %s; "+ + "skipping browser auto-open (open it manually once the tunnel is up)", + url, browserProbeBudget, + ) + return + } + openBrowserAsync(ctx, url) +} + +// probeTCPReachable polls hostPort until a TCP connection succeeds or ctx / +// budget is exhausted. It returns nil on the first successful dial, +// context.DeadlineExceeded when the budget elapses with no listener, or +// context.Canceled when the caller's ctx is cancelled first. +// +// Each dial uses its own short timeout so a hung host can't burn the whole +// budget on one attempt. +func probeTCPReachable( + ctx context.Context, + hostPort string, + budget time.Duration, +) error { + return wait.PollUntilContextTimeout( + ctx, + browserProbeInterval, + budget, + true, // try once at t=0 before the first interval + func(dialCtx context.Context) (bool, error) { + d := net.Dialer{Timeout: browserProbeDial} + conn, err := d.DialContext(dialCtx, "tcp", hostPort) + if err != nil { + return false, nil // keep polling until budget/ctx + } + _ = conn.Close() + return true, nil + }, + ) +} + +// hostPortFromURL extracts a "host:port" suitable for net.DialTimeout from +// a URL string. When the URL has no explicit port the scheme's default +// (80 for http/ws, 443 for https/wss) is used. +// +// Uses u.Hostname() / u.Port() rather than u.Host so IPv6 literals are +// unbracketed for net.JoinHostPort, which adds brackets itself when +// needed. Passing the bracketed u.Host directly produces malformed +// "[[::1]]:80" double-bracketing on the no-port path. +func hostPortFromURL(rawURL string) (string, error) { + u, err := neturl.Parse(rawURL) + if err != nil { + return "", fmt.Errorf("parse url: %w", err) + } + host := u.Hostname() + if host == "" { + return "", fmt.Errorf("url %q has no host", rawURL) + } + port := u.Port() + if port == "" { + port, err = defaultPortForScheme(u.Scheme) + if err != nil { + return "", err + } + } + return net.JoinHostPort(host, port), nil +} + +// defaultPortForScheme returns the well-known port for http/https schemes. +func defaultPortForScheme(scheme string) (string, error) { + switch scheme { + case "http", "ws": + return "80", nil + case "https", "wss": + return "443", nil + default: + return "", fmt.Errorf("unsupported scheme %q (no default port)", scheme) + } } // acquireTunnelLock takes an exclusive file lock that serializes concurrent diff --git a/pkg/ide/opener/browser_tunnel_state_unix_test.go b/pkg/ide/opener/browser_tunnel_state_unix_test.go index 7333a7fb9..6cd374a64 100644 --- a/pkg/ide/opener/browser_tunnel_state_unix_test.go +++ b/pkg/ide/opener/browser_tunnel_state_unix_test.go @@ -208,6 +208,72 @@ func TestLoadLiveTunnelState_LiveMatch(t *testing.T) { } } +func TestTryReuseExistingTunnel_ReturnsRecordedURL(t *testing.T) { + setupTempHome(t) + cmd := spawnSleepHelper(t) + pid := cmd.Process.Pid + + ct, err := helperCreateTime(pid) + if err != nil { + t.Fatalf("helperCreateTime: %v", err) + } + const recordedURL = "http://localhost:10800/?folder=/workspaces/x" + state := TunnelState{ + PID: pid, + CreateTime: ct, + TargetURL: recordedURL, + Label: LabelVSCodeBrowser, + } + if err := WriteTunnelState("ctx", "ws", state); err != nil { + t.Fatalf("WriteTunnelState: %v", err) + } + + // OpenBrowser=false so the test doesn't spawn an actual browser process. + gotURL, ok := tryReuseExistingTunnel( + t.Context(), + "ctx", "ws", + browserIDEInvocation{Label: LabelVSCodeBrowser, OpenBrowser: false}, + ) + if !ok { + t.Fatal("expected tryReuseExistingTunnel to reuse live helper") + } + if gotURL != recordedURL { + t.Errorf("reused URL = %q, want %q", gotURL, recordedURL) + } +} + +func TestTryReuseExistingTunnel_DeadHelperReturnsFalse(t *testing.T) { + setupTempHome(t) + + // Record state with a deliberately wrong identity for PID 1 so the + // match check fails without needing to spawn-and-kill a child. + state := TunnelState{ + PID: 1, + CreateTime: 1, + TargetURL: "http://localhost:10800", + Label: LabelVSCodeBrowser, + } + if err := WriteTunnelState("ctx", "ws", state); err != nil { + t.Fatalf("WriteTunnelState: %v", err) + } + statePath, err := TunnelStateFilePath("ctx", "ws") + if err != nil { + t.Fatalf("TunnelStateFilePath: %v", err) + } + + gotURL, ok := tryReuseExistingTunnel( + t.Context(), + "ctx", "ws", + browserIDEInvocation{Label: LabelVSCodeBrowser, OpenBrowser: false}, + ) + if ok { + t.Errorf("expected reuse=false for non-matching helper, got url=%q", gotURL) + } + if _, err := os.Stat(statePath); !os.IsNotExist(err) { + t.Errorf("expected stale state file removed; stat err=%v", err) + } +} + // parentDir returns filepath.Dir(p). Defined locally rather than importing // path/filepath to keep this test file's import surface minimal. func parentDir(p string) string { diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index d74df6bae..a592b9550 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -1,11 +1,15 @@ package opener import ( + "context" + "errors" + "net" "os" "path/filepath" "slices" "strings" "testing" + "time" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/tunnel" @@ -133,6 +137,162 @@ func assertStatePathSane(t *testing.T, contextName, workspaceID string) { } } +func TestHostPortFromURL(t *testing.T) { + cases := []struct { + name string + in string + want string + wantErr bool + }{ + {"http with port", "http://localhost:10800/x", "localhost:10800", false}, + {"https with port", "https://example.com:8443/", "example.com:8443", false}, + {"http no port", "http://example.com/", "example.com:80", false}, + {"https no port", "https://example.com/", "example.com:443", false}, + {"ipv6 with port", "http://[::1]:10800/", "[::1]:10800", false}, + {"ipv6 no port", "http://[::1]/", "[::1]:80", false}, + {"ipv6 https no port", "https://[2001:db8::1]/", "[2001:db8::1]:443", false}, + {"empty", "", "", true}, + {"no host", "http:///foo", "", true}, + {"unsupported scheme no port", "ftp://example.com/", "", true}, + {"garbage", "://bad", "", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := hostPortFromURL(tc.in) + if tc.wantErr { + if err == nil { + t.Fatalf("hostPortFromURL(%q) = %q, want error", tc.in, got) + } + return + } + if err != nil { + t.Fatalf("hostPortFromURL(%q): %v", tc.in, err) + } + if got != tc.want { + t.Errorf("hostPortFromURL(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestProbeTCPReachable_Reachable(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer func() { _ = ln.Close() }() + + start := time.Now() + if err := probeTCPReachable( + context.Background(), + ln.Addr().String(), + 2*time.Second, + ); err != nil { + t.Fatalf("probeTCPReachable(%s) err = %v; want nil", ln.Addr(), err) + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("probe took %s; expected near-instant success", elapsed) + } +} + +func TestProbeTCPReachable_Unreachable(t *testing.T) { + // Allocate a port, then close the listener so nothing's listening. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + addr := ln.Addr().String() + if err := ln.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + budget := 600 * time.Millisecond + start := time.Now() + err = probeTCPReachable(context.Background(), addr, budget) + elapsed := time.Since(start) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("probeTCPReachable(%s) err = %v; want DeadlineExceeded", addr, err) + } + // Must respect the budget (allow some slack for slow CI). + if elapsed > budget+2*time.Second { + t.Errorf("probe took %s; budget was %s", elapsed, budget) + } +} + +// TestProbeTCPReachable_CtxCancelledBeforeBudget verifies the probe observes +// ctx cancellation and returns context.Canceled so callers can suppress the +// "never came up" warning. +func TestProbeTCPReachable_CtxCancelledBeforeBudget(t *testing.T) { + // Allocate and close a port so dials fail immediately. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + addr := ln.Addr().String() + if err := ln.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + // Cancel from another goroutine shortly after the probe starts so the + // poll loop observes ctx.Done() rather than the budget. + go func() { + time.Sleep(100 * time.Millisecond) + cancel() + }() + + budget := 30 * time.Second + start := time.Now() + err = probeTCPReachable(ctx, addr, budget) + elapsed := time.Since(start) + if !errors.Is(err, context.Canceled) { + t.Fatalf("probeTCPReachable err = %v; want context.Canceled", err) + } + // Must return well before the budget elapses. + if elapsed > 5*time.Second { + t.Errorf("probe took %s; expected to return promptly after ctx cancel", elapsed) + } +} + +// TestOpenBrowserWhenReachable_CtxCancelledSilently verifies that when ctx +// is cancelled before the budget expires no warning is emitted (caller +// already gave up, no need to duplicate the error). +func TestOpenBrowserWhenReachable_CtxCancelledSilently(t *testing.T) { + // Pick a port nothing is listening on. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + addr := ln.Addr().String() + if err := ln.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(100 * time.Millisecond) + cancel() + }() + + done := make(chan struct{}) + start := time.Now() + go func() { + openBrowserWhenReachable(ctx, "http://"+addr) + close(done) + }() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatalf("openBrowserWhenReachable did not return after ctx cancel") + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf( + "openBrowserWhenReachable returned after %s; expected prompt return on ctx cancel", + elapsed, + ) + } +} + func TestReadTunnelState_MissingReturnsNilNil(t *testing.T) { setupTempHome(t) diff --git a/pkg/ide/opener/opener.go b/pkg/ide/opener/opener.go index 96631ba3a..62c32292d 100644 --- a/pkg/ide/opener/opener.go +++ b/pkg/ide/opener/opener.go @@ -298,7 +298,7 @@ func openJupyterBrowser( pkglog.Infof("Starting jupyter notebook in browser mode at %s", targetURL) extraPorts := []string{fmt.Sprintf("%s:%d", addr, jupyter.DefaultServerPort)} - if err := startDetachedBrowserTunnel(ctx, params, tunnel.BrowserTunnelParams{ + effectiveURL, err := startDetachedBrowserTunnel(ctx, params, tunnel.BrowserTunnelParams{ DevsyConfig: params.DevsyConfig, Client: params.Client, User: params.User, @@ -307,10 +307,11 @@ func openJupyterBrowser( AuthSockID: params.SSHAuthSockID, GitSSHSigningKey: params.GitSSHSigningKey, DaemonStartFunc: makeDaemonStartFunc(params, false, extraPorts), - }, browserIDEInvocation{Label: "jupyter", OpenBrowser: openBrowser}); err != nil { + }, browserIDEInvocation{Label: "jupyter", OpenBrowser: openBrowser}) + if err != nil { return "", err } - return targetURL, nil + return effectiveURL, nil } func openRStudioBrowser( @@ -337,7 +338,7 @@ func openRStudioBrowser( pkglog.Infof("Starting RStudio server in browser mode at %s", targetURL) extraPorts := []string{fmt.Sprintf("%s:%d", addr, rstudio.DefaultServerPort)} - if err := startDetachedBrowserTunnel(ctx, params, tunnel.BrowserTunnelParams{ + effectiveURL, err := startDetachedBrowserTunnel(ctx, params, tunnel.BrowserTunnelParams{ DevsyConfig: params.DevsyConfig, Client: params.Client, User: params.User, @@ -346,10 +347,11 @@ func openRStudioBrowser( AuthSockID: params.SSHAuthSockID, GitSSHSigningKey: params.GitSSHSigningKey, DaemonStartFunc: makeDaemonStartFunc(params, false, extraPorts), - }, browserIDEInvocation{Label: "rstudio", OpenBrowser: openBrowser}); err != nil { + }, browserIDEInvocation{Label: "rstudio", OpenBrowser: openBrowser}) + if err != nil { return "", err } - return targetURL, nil + return effectiveURL, nil } func openVSCodeBrowser( @@ -381,7 +383,7 @@ func openVSCodeBrowser( openvscode.ForwardPortsOption, ) == config.BoolTrue extraPorts := []string{fmt.Sprintf("%s:%d", addr, openvscode.DefaultVSCodePort)} - if err := startDetachedBrowserTunnel(ctx, params, tunnel.BrowserTunnelParams{ + effectiveURL, err := startDetachedBrowserTunnel(ctx, params, tunnel.BrowserTunnelParams{ DevsyConfig: params.DevsyConfig, Client: params.Client, User: params.User, @@ -391,10 +393,11 @@ func openVSCodeBrowser( AuthSockID: params.SSHAuthSockID, GitSSHSigningKey: params.GitSSHSigningKey, DaemonStartFunc: makeDaemonStartFunc(params, forwardPorts, extraPorts), - }, browserIDEInvocation{Label: LabelVSCodeBrowser, OpenBrowser: openBrowser}); err != nil { + }, browserIDEInvocation{Label: LabelVSCodeBrowser, OpenBrowser: openBrowser}) + if err != nil { return "", err } - return targetURL, nil + return effectiveURL, nil } func startFleet(ctx context.Context, params IDEParams) (string, error) {