From 207d79a801bed9dda177bf198b0f306492f8a48f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 08:18:59 -0500 Subject: [PATCH 1/5] fix(ide): return reused tunnel URL and auto-open browser via daemon Two regressions from PR #436 surfaced when reopening a browser IDE for a workspace that already had a detached helper running: - openVSCodeBrowser/Jupyter/RStudio computed targetURL via port.FindAvailablePort BEFORE startDetachedBrowserTunnel decided whether to reuse the existing helper. When reuse triggered, the freshly allocated port (e.g. 10801) was returned in the result envelope while the helper was actually listening on the recorded port (10800). The "Open IDE" link in the desktop then pointed at a port nothing was bound to. Fix: have startDetachedBrowserTunnel return the effective URL (the reused helper's TargetURL when reuse fires, the freshly-spawned helper's URL otherwise) and use that as the opener return value. - The daemon-client early return in startDetachedBrowserTunnel bypassed openBrowserAsync, so workspaces launched through the desktop's local daemon never popped a browser window. Fix: kick off openBrowserAsync before delegating to tunnel.StartBrowserTunnel when inv.OpenBrowser is set. --- pkg/ide/opener/browser_tunnel.go | 38 ++++++----- .../opener/browser_tunnel_state_unix_test.go | 66 +++++++++++++++++++ pkg/ide/opener/opener.go | 21 +++--- 3 files changed, 100 insertions(+), 25 deletions(-) diff --git a/pkg/ide/opener/browser_tunnel.go b/pkg/ide/opener/browser_tunnel.go index 5418e28ba..0fac3eae2 100644 --- a/pkg/ide/opener/browser_tunnel.go +++ b/pkg/ide/opener/browser_tunnel.go @@ -154,17 +154,22 @@ 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) + if openBrowser { + go openBrowserAsync(ctx, tunnelParams.TargetURL) + } + return tunnelParams.TargetURL, tunnel.StartBrowserTunnel(ctx, tunnelParams) } contextName := params.Client.Context() @@ -174,7 +179,7 @@ func startDetachedBrowserTunnel( // 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 +187,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 +206,7 @@ func startDetachedBrowserTunnel( go openBrowserAsync(ctx, tunnelParams.TargetURL) } - return nil + return tunnelParams.TargetURL, nil } // spawnTunnelHelper locates the current executable, builds the helper @@ -400,18 +405,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 +427,12 @@ 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 } // 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/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) { From fc30e6db36d51247db6d7303c8cd181efc26012e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 07:44:06 -0500 Subject: [PATCH 2/5] fix(ide): probe URL and reuse helper in daemon browser-tunnel path The daemon-client branch of startDetachedBrowserTunnel had two bugs: 1. It launched the browser concurrently with tunnel.StartBrowserTunnel, so a fast-failing tunnel (port-bind error, daemon unreachable) would still pop a tab pointed at a dead address. Now the auto-open goroutine probes the target URL's TCP port (up to ~5s, 200ms ticks, 300ms dial timeout) before calling open.Open; if the port never becomes reachable a warning is logged instead. 2. It never checked for an existing live helper, so a workspace with a helper already running on a different port would still spawn a new tunnel and open the browser to the wrong URL. The daemon branch now acquires the same tunnel lock and consults tryReuseExistingTunnel, matching the non-daemon path. Adds unit tests for the URL parsing helper (http/https with and without explicit ports, ipv6, error cases) and for probeTCPReachable against both a live and a closed listener. --- pkg/ide/opener/browser_tunnel.go | 139 ++++++++++++++++++++++++-- pkg/ide/opener/browser_tunnel_test.go | 79 +++++++++++++++ 2 files changed, 211 insertions(+), 7 deletions(-) diff --git a/pkg/ide/opener/browser_tunnel.go b/pkg/ide/opener/browser_tunnel.go index 0fac3eae2..69b52464e 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" @@ -18,6 +20,14 @@ import ( "github.com/gofrs/flock" ) +// 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 // (if any) and removes its state file. Missing state and dead/foreign PIDs // are tolerated silently; lock contention is logged at Warn level. Safe to @@ -165,16 +175,19 @@ func startDetachedBrowserTunnel( ) (string, error) { label := inv.Label openBrowser := inv.OpenBrowser - if _, ok := params.Client.(client.DaemonClient); ok { - if openBrowser { - go openBrowserAsync(ctx, tunnelParams.TargetURL) - } - return tunnelParams.TargetURL, 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) @@ -435,6 +448,118 @@ func tryReuseExistingTunnel( 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 + } + + if a.OpenBrowser { + go openBrowserWhenReachable(ctx, 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 the port never becomes reachable +// within browserProbeBudget the browser is NOT opened and a warning is +// logged — better than launching a tab to a dead address when +// tunnel.StartBrowserTunnel fails fast. +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 !probeTCPReachable(hostPort, browserProbeBudget) { + 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 returns true if a TCP connection to hostPort succeeds +// within the given budget, polling every browserProbeInterval. Each dial +// uses a short timeout so a hung host can't burn the whole budget on one +// attempt. +func probeTCPReachable(hostPort string, budget time.Duration) bool { + deadline := time.Now().Add(budget) + for { + conn, err := net.DialTimeout("tcp", hostPort, browserProbeDial) + if err == nil { + _ = conn.Close() + return true + } + if time.Now().After(deadline) { + return false + } + time.Sleep(browserProbeInterval) + } +} + +// 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, 443 for https) is used. +func hostPortFromURL(rawURL string) (string, error) { + u, err := neturl.Parse(rawURL) + if err != nil { + return "", fmt.Errorf("parse url: %w", err) + } + if u.Host == "" { + return "", fmt.Errorf("url %q has no host", rawURL) + } + host, port, splitErr := net.SplitHostPort(u.Host) + if splitErr == nil { + if host == "" || port == "" { + return "", fmt.Errorf("url %q has empty host or port", rawURL) + } + return net.JoinHostPort(host, port), nil + } + // No explicit port; fall back to scheme default. + defaultPort, err := defaultPortForScheme(u.Scheme) + if err != nil { + return "", err + } + return net.JoinHostPort(u.Host, defaultPort), 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 // attempts to start (or reuse) the browser tunnel for a workspace. The // returned function releases the lock. diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index d74df6bae..5583f7feb 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -1,11 +1,13 @@ package opener import ( + "net" "os" "path/filepath" "slices" "strings" "testing" + "time" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/tunnel" @@ -133,6 +135,83 @@ 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}, + {"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() + ok := probeTCPReachable(ln.Addr().String(), 2*time.Second) + if !ok { + t.Fatalf("probeTCPReachable(%s) = false; want true", ln.Addr()) + } + 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() + ok := probeTCPReachable(addr, budget) + elapsed := time.Since(start) + if ok { + t.Fatalf("probeTCPReachable(%s) = true; want false (nothing listening)", addr) + } + // 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) + } +} + func TestReadTunnelState_MissingReturnsNilNil(t *testing.T) { setupTempHome(t) From 83db77030a342aa104c9971e73916debf44470ea Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 08:15:56 -0500 Subject: [PATCH 3/5] fix(ide/opener): cancel probe goroutine when daemon tunnel exits runDaemonBrowserTunnel previously left openBrowserWhenReachable polling up to its 5s budget even after tunnel.StartBrowserTunnel returned an error, surfacing a misleading "URL never became reachable" warning next to the real failure. Pass a derived context to the probe, cancel it on function return, and have the probe distinguish ctx cancellation (silent) from budget expiry (warn). --- pkg/ide/opener/browser_tunnel.go | 53 +++++++++++----- pkg/ide/opener/browser_tunnel_test.go | 88 ++++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 16 deletions(-) diff --git a/pkg/ide/opener/browser_tunnel.go b/pkg/ide/opener/browser_tunnel.go index 69b52464e..f02b5dbb4 100644 --- a/pkg/ide/opener/browser_tunnel.go +++ b/pkg/ide/opener/browser_tunnel.go @@ -475,24 +475,37 @@ func runDaemonBrowserTunnel(ctx context.Context, a daemonTunnelArgs) (string, er 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(ctx, a.TunnelParams.TargetURL) + 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 the port never becomes reachable -// within browserProbeBudget the browser is NOT opened and a warning is -// logged — better than launching a tab to a dead address when -// tunnel.StartBrowserTunnel fails fast. +// 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 !probeTCPReachable(hostPort, browserProbeBudget) { + reachable, cancelled := probeTCPReachable(ctx, hostPort, browserProbeBudget) + if cancelled { + return + } + if !reachable { pkglog.Warnf( "browser-tunnel URL %s never became reachable within %s; "+ "skipping browser auto-open (open it manually once the tunnel is up)", @@ -503,22 +516,34 @@ func openBrowserWhenReachable(ctx context.Context, url string) { openBrowserAsync(ctx, url) } -// probeTCPReachable returns true if a TCP connection to hostPort succeeds -// within the given budget, polling every browserProbeInterval. Each dial -// uses a short timeout so a hung host can't burn the whole budget on one -// attempt. -func probeTCPReachable(hostPort string, budget time.Duration) bool { +// probeTCPReachable returns (reachable, cancelled). reachable=true means a +// TCP dial succeeded within the budget. cancelled=true means ctx was +// cancelled before either success or budget expiry; in that case reachable +// is false and the caller should NOT log a "never came up" warning. +// Each dial uses a 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, +) (reachable bool, cancelled bool) { deadline := time.Now().Add(budget) + ticker := time.NewTicker(browserProbeInterval) + defer ticker.Stop() for { conn, err := net.DialTimeout("tcp", hostPort, browserProbeDial) if err == nil { _ = conn.Close() - return true + return true, false } if time.Now().After(deadline) { - return false + return false, false + } + select { + case <-ctx.Done(): + return false, true + case <-ticker.C: } - time.Sleep(browserProbeInterval) } } diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index 5583f7feb..be936982d 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -1,6 +1,7 @@ package opener import ( + "context" "net" "os" "path/filepath" @@ -179,10 +180,13 @@ func TestProbeTCPReachable_Reachable(t *testing.T) { defer func() { _ = ln.Close() }() start := time.Now() - ok := probeTCPReachable(ln.Addr().String(), 2*time.Second) + ok, cancelled := probeTCPReachable(context.Background(), ln.Addr().String(), 2*time.Second) if !ok { t.Fatalf("probeTCPReachable(%s) = false; want true", ln.Addr()) } + if cancelled { + t.Fatalf("probeTCPReachable cancelled=true; want false") + } if elapsed := time.Since(start); elapsed > time.Second { t.Errorf("probe took %s; expected near-instant success", elapsed) } @@ -201,17 +205,97 @@ func TestProbeTCPReachable_Unreachable(t *testing.T) { budget := 600 * time.Millisecond start := time.Now() - ok := probeTCPReachable(addr, budget) + ok, cancelled := probeTCPReachable(context.Background(), addr, budget) elapsed := time.Since(start) if ok { t.Fatalf("probeTCPReachable(%s) = true; want false (nothing listening)", addr) } + if cancelled { + t.Fatalf("probeTCPReachable cancelled=true; want false (budget expired)") + } // 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 loop +// observes ctx cancellation and reports cancelled=true 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 + // loop hits the ctx.Done() branch rather than the budget branch. + go func() { + time.Sleep(100 * time.Millisecond) + cancel() + }() + + budget := 30 * time.Second + start := time.Now() + ok, cancelled := probeTCPReachable(ctx, addr, budget) + elapsed := time.Since(start) + if ok { + t.Fatalf("probeTCPReachable = reachable; want false") + } + if !cancelled { + t.Fatalf("probeTCPReachable cancelled=false; want true on ctx cancel") + } + // 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) From afc0a0235112f945772d53af4db46a2d4354ba21 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 09:48:06 -0500 Subject: [PATCH 4/5] refactor(ide/opener): use wait.PollUntilContextTimeout for tcp probe Replaces the hand-rolled ticker/deadline loop in probeTCPReachable with k8s.io/apimachinery/pkg/util/wait.PollUntilContextTimeout, an existing dependency already used in cmd/pro, pkg/agent/inject, pkg/credentials, and elsewhere. The library does the cancel/timeout/poll interleave so the per-call logic shrinks to "dial once, report (true, nil) on success". API change: probeTCPReachable now returns error (nil on reachable, context.DeadlineExceeded on budget timeout, context.Canceled on outer ctx cancel) instead of (reachable, cancelled bool). openBrowserWhenReachable discriminates via errors.Is(err, context.Canceled) to keep the existing "silent on cancel, warn on budget expiry" behavior. --- pkg/ide/opener/browser_tunnel.go | 54 +++++++++++++-------------- pkg/ide/opener/browser_tunnel_test.go | 39 +++++++++---------- 2 files changed, 43 insertions(+), 50 deletions(-) diff --git a/pkg/ide/opener/browser_tunnel.go b/pkg/ide/opener/browser_tunnel.go index f02b5dbb4..8a604d669 100644 --- a/pkg/ide/opener/browser_tunnel.go +++ b/pkg/ide/opener/browser_tunnel.go @@ -18,6 +18,7 @@ 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 @@ -501,11 +502,10 @@ func openBrowserWhenReachable(ctx context.Context, url string) { pkglog.Warnf("could not parse browser-tunnel URL %q: %v; skipping auto-open", url, err) return } - reachable, cancelled := probeTCPReachable(ctx, hostPort, browserProbeBudget) - if cancelled { - return - } - if !reachable { + 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)", @@ -516,35 +516,33 @@ func openBrowserWhenReachable(ctx context.Context, url string) { openBrowserAsync(ctx, url) } -// probeTCPReachable returns (reachable, cancelled). reachable=true means a -// TCP dial succeeded within the budget. cancelled=true means ctx was -// cancelled before either success or budget expiry; in that case reachable -// is false and the caller should NOT log a "never came up" warning. -// Each dial uses a short timeout so a hung host can't burn the whole +// 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, -) (reachable bool, cancelled bool) { - deadline := time.Now().Add(budget) - ticker := time.NewTicker(browserProbeInterval) - defer ticker.Stop() - for { - conn, err := net.DialTimeout("tcp", hostPort, browserProbeDial) - if err == nil { +) 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, false - } - if time.Now().After(deadline) { - return false, false - } - select { - case <-ctx.Done(): - return false, true - case <-ticker.C: - } - } + return true, nil + }, + ) } // hostPortFromURL extracts a "host:port" suitable for net.DialTimeout from diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index be936982d..aa1cd406b 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -2,6 +2,7 @@ package opener import ( "context" + "errors" "net" "os" "path/filepath" @@ -180,12 +181,12 @@ func TestProbeTCPReachable_Reachable(t *testing.T) { defer func() { _ = ln.Close() }() start := time.Now() - ok, cancelled := probeTCPReachable(context.Background(), ln.Addr().String(), 2*time.Second) - if !ok { - t.Fatalf("probeTCPReachable(%s) = false; want true", ln.Addr()) - } - if cancelled { - t.Fatalf("probeTCPReachable cancelled=true; want false") + 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) @@ -205,13 +206,10 @@ func TestProbeTCPReachable_Unreachable(t *testing.T) { budget := 600 * time.Millisecond start := time.Now() - ok, cancelled := probeTCPReachable(context.Background(), addr, budget) + err = probeTCPReachable(context.Background(), addr, budget) elapsed := time.Since(start) - if ok { - t.Fatalf("probeTCPReachable(%s) = true; want false (nothing listening)", addr) - } - if cancelled { - t.Fatalf("probeTCPReachable cancelled=true; want false (budget expired)") + 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 { @@ -219,9 +217,9 @@ func TestProbeTCPReachable_Unreachable(t *testing.T) { } } -// TestProbeTCPReachable_CtxCancelledBeforeBudget verifies the probe loop -// observes ctx cancellation and reports cancelled=true so callers can -// suppress the "never came up" warning. +// 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") @@ -235,7 +233,7 @@ func TestProbeTCPReachable_CtxCancelledBeforeBudget(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) // Cancel from another goroutine shortly after the probe starts so the - // loop hits the ctx.Done() branch rather than the budget branch. + // poll loop observes ctx.Done() rather than the budget. go func() { time.Sleep(100 * time.Millisecond) cancel() @@ -243,13 +241,10 @@ func TestProbeTCPReachable_CtxCancelledBeforeBudget(t *testing.T) { budget := 30 * time.Second start := time.Now() - ok, cancelled := probeTCPReachable(ctx, addr, budget) + err = probeTCPReachable(ctx, addr, budget) elapsed := time.Since(start) - if ok { - t.Fatalf("probeTCPReachable = reachable; want false") - } - if !cancelled { - t.Fatalf("probeTCPReachable cancelled=false; want true on ctx cancel") + 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 { From 3e56654fa99576de254de4e252f1a556fdd844b2 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 11:44:18 -0500 Subject: [PATCH 5/5] fix(ide/opener): unbracket IPv6 hostnames before net.JoinHostPort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hostPortFromURL re-bracketed already-bracketed IPv6 literals on the no-explicit-port fallback. For url.Parse("http://[::1]/"), u.Host is "[::1]", and net.JoinHostPort("[::1]", "80") returns "[[::1]]:80" — malformed and unusable for net.DialTimeout. Switch to u.Hostname() and u.Port(), which return the unbracketed host and explicit port (or "") respectively. net.JoinHostPort then adds brackets exactly once when the host contains colons. Two cross-reviewer agents independently verified the bug by running the actual Go stdlib calls; the existing test only covered IPv6 with an explicit port, missing the broken path. New cases: - http://[::1]/ → [::1]:80 - https://[2001:db8::1]/ → [2001:db8::1]:443 Practical impact today is low (Devsy's tunnel URLs always carry an explicit port), but the latent correctness gap is closed and the function is shorter and locale-free. --- pkg/ide/opener/browser_tunnel.go | 27 ++++++++++++++------------- pkg/ide/opener/browser_tunnel_test.go | 2 ++ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/pkg/ide/opener/browser_tunnel.go b/pkg/ide/opener/browser_tunnel.go index 8a604d669..0cfffd3bf 100644 --- a/pkg/ide/opener/browser_tunnel.go +++ b/pkg/ide/opener/browser_tunnel.go @@ -547,28 +547,29 @@ func probeTCPReachable( // 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, 443 for https) is used. +// (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) } - if u.Host == "" { + host := u.Hostname() + if host == "" { return "", fmt.Errorf("url %q has no host", rawURL) } - host, port, splitErr := net.SplitHostPort(u.Host) - if splitErr == nil { - if host == "" || port == "" { - return "", fmt.Errorf("url %q has empty host or port", rawURL) + port := u.Port() + if port == "" { + port, err = defaultPortForScheme(u.Scheme) + if err != nil { + return "", err } - return net.JoinHostPort(host, port), nil - } - // No explicit port; fall back to scheme default. - defaultPort, err := defaultPortForScheme(u.Scheme) - if err != nil { - return "", err } - return net.JoinHostPort(u.Host, defaultPort), nil + return net.JoinHostPort(host, port), nil } // defaultPortForScheme returns the well-known port for http/https schemes. diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index aa1cd406b..a592b9550 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -149,6 +149,8 @@ func TestHostPortFromURL(t *testing.T) { {"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},