From a13aff7e791aaa8d44bcf7a0fbed1428d9e1e871 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 02:50:03 +0000 Subject: [PATCH 01/10] fix(agentcontainer): treat credentials-server port race as a no-op When a workspace is opened via IDE, two client-side processes race to run `credentials-server` inside the same devcontainer on the fixed port: the background services daemon started on workspace open, and the interactive `devsy ssh` session started when a terminal connects. Only one session's credentials-server can hold the port at a time by design; losing is expected, not a failure. Previously claimPort's EADDRINUSE failure was returned as a generic error, so the losing session's client-side retry.OnError loop kept retrying with exponential backoff (up to ~17 minutes), and each attempt logged an ERROR-level line. ee31e61f8 fixed a JSON double-logging bug that had been accidentally hiding these lines at Debug level, which surfaced this retry storm directly in the user's SSH terminal on every workspace connect, making it hard to type. Wrap EADDRINUSE in a distinct errPortOwnedByAnotherSession sentinel and short-circuit Run to log once at Debug and exit cleanly (0) instead of erroring, so the retry loop never fires and nothing is logged to the terminal by default. --- .../agentcontainer/credentials_server.go | 37 +++++++++++++------ .../agentcontainer/credentials_server_test.go | 21 ++++++++++- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/cmd/internal/agentcontainer/credentials_server.go b/cmd/internal/agentcontainer/credentials_server.go index 35ae78ac5..31da92031 100644 --- a/cmd/internal/agentcontainer/credentials_server.go +++ b/cmd/internal/agentcontainer/credentials_server.go @@ -4,10 +4,12 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "net" "os" "strconv" + "syscall" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/agent/tunnel" @@ -91,6 +93,19 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { runCtx, cancel := context.WithCancel(ctx) defer cancel() + ln, err := claimPort(port) + if err != nil { + if errors.Is(err, errPortOwnedByAnotherSession) { + log.Debugf( + "skipping credentials server: %v (another session already provides it for this container)", + err, + ) + return nil + } + return err + } + defer func() { _ = ln.Close() }() + tunnelClient, err := tunnelserver.NewTunnelClient(os.Stdin, os.Stdout, true, ExitCodeIO) if err != nil { return fmt.Errorf("error creating tunnel client: %w", err) @@ -100,12 +115,6 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { return fmt.Errorf("ping client: %w", err) } - ln, err := claimPort(port) - if err != nil { - return err - } - defer func() { _ = ln.Close() }() - cmd.maybeForwardPorts(runCtx, tunnelClient) if err := cmd.configureDockerHelper(port); err != nil { @@ -129,6 +138,13 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { return credentials.RunCredentialsServerWithListener(runCtx, ln, tunnelClient) } +// errPortOwnedByAnotherSession indicates the credentials-server port is +// already bound, almost certainly by another session's credentials-server +// for this same container. Only one session's credentials-server can hold +// this port at a time; losing the race is expected, not a failure, so +// callers should treat it as a no-op rather than an error. +var errPortOwnedByAnotherSession = errors.New("credentials server port owned by another session") + // claimPort binds port and returns the listener, holding it exclusively so // no other session can bind the same port until the caller closes it (or // hands it to RunCredentialsServerWithListener). Only one session's @@ -137,11 +153,10 @@ func claimPort(port int) (net.Listener, error) { addr := net.JoinHostPort("localhost", strconv.Itoa(port)) ln, err := net.Listen("tcp", addr) if err != nil { - return nil, fmt.Errorf( - "port %d not available (another session may own the credentials server): %w", - port, - err, - ) + if errors.Is(err, syscall.EADDRINUSE) { + return nil, fmt.Errorf("%w: %w", errPortOwnedByAnotherSession, err) + } + return nil, fmt.Errorf("port %d not available: %w", port, err) } return ln, nil } diff --git a/cmd/internal/agentcontainer/credentials_server_test.go b/cmd/internal/agentcontainer/credentials_server_test.go index a4aa77b3b..9d174a9bc 100644 --- a/cmd/internal/agentcontainer/credentials_server_test.go +++ b/cmd/internal/agentcontainer/credentials_server_test.go @@ -1,6 +1,8 @@ package agentcontainer import ( + "context" + "errors" "net" "sync" "testing" @@ -23,7 +25,7 @@ func TestClaimPort_ErrorsWhenPortHeld(t *testing.T) { _, err = claimPort(port) require.Error(t, err) - assert.Contains(t, err.Error(), "not available") + assert.ErrorIs(t, err, errPortOwnedByAnotherSession) } func TestClaimPort_BecomesClaimableAfterHolderReleases(t *testing.T) { @@ -79,3 +81,20 @@ func TestClaimPort_OnlyOneConcurrentCallerWins(t *testing.T) { _ = winner.Close() } } + +func TestCredentialsServerCmd_Run_TreatsPortOwnedByAnotherSessionAsNoOp(t *testing.T) { + ln, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + t.Cleanup(func() { _ = ln.Close() }) + port := ln.Addr().(*net.TCPAddr).Port + + cmd := &CredentialsServerCmd{} + err = cmd.Run(context.Background(), port) + require.NoError(t, err, "losing the port claim to another session must not be an error") +} + +func TestClaimPort_WrapsNonAddrInUseErrorsWithoutSentinel(t *testing.T) { + _, err := claimPort(-1) + require.Error(t, err) + assert.False(t, errors.Is(err, errPortOwnedByAnotherSession)) +} From 04627f951cf7c0c23f99a7fd9d6115f734ce550b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 03:03:31 +0000 Subject: [PATCH 02/10] fix(credentials): distinguish same-user vs cross-user port collisions The short-term fix (previous commit) silently no-ops any port claim loss, which is correct when the winning session serves the same container user (redundant, harmless) but silently wrong when it serves a different one: that user's git/docker/signing credential helpers never get configured, with no signal that anything is missing, since claimPort/configureGitCredentialHelper/etc are all keyed by cmd.User rather than by the shared, fixed 12049 port. Expose an /owner endpoint on the credentials-server HTTP handler reporting which container user it was started for. A session that loses the port claim now calls credentials.FetchOwner to look up the winner's user before deciding how to react: - same user (or owner unknown, e.g. an older binary without this endpoint) -> Debug log, silent no-op, as before. - different user -> Warn log naming both users, still returns nil (retrying wouldn't help; the other session isn't going away). This keeps the common case (IDE opener daemon and an interactive devsy ssh session run as the same container user) completely silent while making the previously-invisible cross-user gap loud instead of silently swallowed. --- .../agentcontainer/credentials_server.go | 42 +++++++- .../agentcontainer/credentials_server_test.go | 102 +++++++++++++++++- pkg/credentials/server.go | 65 ++++++++++- pkg/credentials/server_test.go | 48 +++++++++ pkg/credentials/start.go | 2 +- 5 files changed, 245 insertions(+), 14 deletions(-) diff --git a/cmd/internal/agentcontainer/credentials_server.go b/cmd/internal/agentcontainer/credentials_server.go index 31da92031..267ae5b70 100644 --- a/cmd/internal/agentcontainer/credentials_server.go +++ b/cmd/internal/agentcontainer/credentials_server.go @@ -96,10 +96,7 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { ln, err := claimPort(port) if err != nil { if errors.Is(err, errPortOwnedByAnotherSession) { - log.Debugf( - "skipping credentials server: %v (another session already provides it for this container)", - err, - ) + cmd.logPortOwnedByAnotherSession(ctx, port) return nil } return err @@ -135,7 +132,7 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { cleanupGitSigning := cmd.configureGitSigningKey() defer cleanupGitSigning() - return credentials.RunCredentialsServerWithListener(runCtx, ln, tunnelClient) + return credentials.RunCredentialsServerWithListener(runCtx, ln, tunnelClient, cmd.User) } // errPortOwnedByAnotherSession indicates the credentials-server port is @@ -161,6 +158,41 @@ func claimPort(port int) (net.Listener, error) { return ln, nil } +// logPortOwnedByAnotherSession reports why this session is skipping +// credentials-server setup after losing the port claim. It fetches the +// owner of the server that won the race to tell apart the two outcomes: +// the same container user already has a working credentials server +// (redundant and harmless), versus a different user's session owns it, in +// which case cmd.User's own git/docker/signing helpers were never +// configured and won't work until that other session ends. +func (cmd *CredentialsServerCmd) logPortOwnedByAnotherSession(ctx context.Context, port int) { + owner, err := credentials.FetchOwner(ctx, port) + switch { + case err != nil: + log.Debugf( + "skipping credentials server for user %s: port %d is taken and its owner could not be determined: %v", + cmd.User, + port, + err, + ) + case owner == "" || owner == cmd.User: + log.Debugf( + "skipping credentials server for user %s: another session already provides it on port %d", + cmd.User, + port, + ) + default: + log.Warnf( + "credentials server for user %s was not started: port %d is already owned by user %s's session; "+ + "git/docker/signing credential helpers for %s will not work until that session ends", + cmd.User, + port, + owner, + cmd.User, + ) + } +} + func (cmd *CredentialsServerCmd) maybeForwardPorts( ctx context.Context, tunnelClient tunnel.TunnelClient, diff --git a/cmd/internal/agentcontainer/credentials_server_test.go b/cmd/internal/agentcontainer/credentials_server_test.go index 9d174a9bc..6d755f024 100644 --- a/cmd/internal/agentcontainer/credentials_server_test.go +++ b/cmd/internal/agentcontainer/credentials_server_test.go @@ -3,12 +3,19 @@ package agentcontainer import ( "context" "errors" + "fmt" "net" + "strings" "sync" "testing" + "time" + "github.com/devsy-org/devsy/pkg/agent/tunnel" + "github.com/devsy-org/devsy/pkg/credentials" + "github.com/devsy-org/devsy/pkg/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc" ) func TestClaimPort_SucceedsWhenPortFree(t *testing.T) { @@ -82,15 +89,100 @@ func TestClaimPort_OnlyOneConcurrentCallerWins(t *testing.T) { } } -func TestCredentialsServerCmd_Run_TreatsPortOwnedByAnotherSessionAsNoOp(t *testing.T) { +// startFakeCredentialsServer binds an ephemeral port and serves it as the +// given owner, simulating the session that wins the credentials-server port +// claim race. It returns the port once the server is dialable. +func startFakeCredentialsServer(t *testing.T, owner string) int { + t.Helper() + ln, err := net.Listen("tcp", "localhost:0") require.NoError(t, err) - t.Cleanup(func() { _ = ln.Close() }) port := ln.Addr().(*net.TCPAddr).Port - cmd := &CredentialsServerCmd{} - err = cmd.Run(context.Background(), port) - require.NoError(t, err, "losing the port claim to another session must not be an error") + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { + _ = credentials.RunCredentialsServerWithListener(ctx, ln, &fakeCredentialsClient{}, owner) + }() + + require.Eventually(t, func() bool { + conn, dialErr := net.Dial("tcp", ln.Addr().String()) + if dialErr != nil { + return false + } + _ = conn.Close() + return true + }, time.Second, 5*time.Millisecond, "fake credentials server must become dialable") + + return port +} + +func TestCredentialsServerCmd_Run_SameOwnerCollisionIsSilentNoOp(t *testing.T) { + port := startFakeCredentialsServer(t, "alice") + + var sink strings.Builder + log.Init(log.Config{Verbosity: 2, Format: "json"}) + remove := log.AddSink(&sink) + defer remove() + + cmd := &CredentialsServerCmd{User: "alice"} + err := cmd.Run(context.Background(), port) + require.NoError(t, err, "losing to the same owner's session must not be an error") + _ = log.Sync() + + assert.NotContains(t, sink.String(), "\"level\":\"warn\"", "same-owner collision must not warn") +} + +func TestCredentialsServerCmd_Run_DifferentOwnerCollisionWarnsButDoesNotError(t *testing.T) { + port := startFakeCredentialsServer(t, "alice") + + var sink strings.Builder + log.Init(log.Config{Verbosity: 2, Format: "json"}) + remove := log.AddSink(&sink) + defer remove() + + cmd := &CredentialsServerCmd{User: "root"} + err := cmd.Run(context.Background(), port) + require.NoError(t, err, "losing the race must still not fail the session") + _ = log.Sync() + + logged := sink.String() + assert.Contains(t, logged, "root", "warning must name the user left without credentials") + assert.Contains(t, logged, "alice", "warning must name the owning session") +} + +// fakeCredentialsClient satisfies credentials.CredentialsClient for tests +// that only need a listening server, not real request handling. +type fakeCredentialsClient struct{} + +func (fakeCredentialsClient) GitCredentials( + _ context.Context, _ *tunnel.Message, _ ...grpc.CallOption, +) (*tunnel.Message, error) { + return nil, fmt.Errorf("not implemented") +} + +func (fakeCredentialsClient) DockerCredentials( + _ context.Context, _ *tunnel.Message, _ ...grpc.CallOption, +) (*tunnel.Message, error) { + return nil, fmt.Errorf("not implemented") +} + +func (fakeCredentialsClient) GitSSHSignature( + _ context.Context, _ *tunnel.Message, _ ...grpc.CallOption, +) (*tunnel.Message, error) { + return nil, fmt.Errorf("not implemented") +} + +func (fakeCredentialsClient) GPGPublicKeys( + _ context.Context, _ *tunnel.Message, _ ...grpc.CallOption, +) (*tunnel.Message, error) { + return nil, fmt.Errorf("not implemented") +} + +func (fakeCredentialsClient) DevsyConfig( + _ context.Context, _ *tunnel.Message, _ ...grpc.CallOption, +) (*tunnel.Message, error) { + return nil, fmt.Errorf("not implemented") } func TestClaimPort_WrapsNonAddrInUseErrorsWithoutSentinel(t *testing.T) { diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index 9858e2a86..222de6921 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "strconv" + "strings" "time" "github.com/devsy-org/devsy/pkg/agent/tunnel" @@ -44,25 +45,33 @@ func RunCredentialsServer( ctx context.Context, port int, client CredentialsClient, + owner string, ) error { ln, err := net.Listen("tcp", net.JoinHostPort("localhost", strconv.Itoa(port))) if err != nil { return fmt.Errorf("listen on port %d: %w", port, err) } - return RunCredentialsServerWithListener(ctx, ln, client) + return RunCredentialsServerWithListener(ctx, ln, client, owner) } // RunCredentialsServerWithListener is like RunCredentialsServer, but takes an // already-bound listener. Use this when the caller must hold the port // exclusively (via net.Listen) from before startup through to serving, so no // other process can bind the same port in between. +// +// owner identifies who this server is serving (e.g. the container user that +// configured it), exposed on ownerPath so a session that loses the port +// claim can tell whether skipping is safe (same owner: redundant, safe to +// skip) or not (different owner: that owner's credential helpers were never +// configured). func RunCredentialsServerWithListener( ctx context.Context, ln net.Listener, client CredentialsClient, + owner string, ) error { srv := &http.Server{ - Handler: newCredentialsHandler(ctx, client), + Handler: newCredentialsHandler(ctx, client, owner), ReadHeaderTimeout: 10 * time.Second, ReadTimeout: 30 * time.Second, IdleTimeout: 120 * time.Second, @@ -91,18 +100,33 @@ type credentialsHandlerFunc func( context.Context, http.ResponseWriter, *http.Request, CredentialsClient, ) error +// ownerPath serves the owner string RunCredentialsServerWithListener was +// started with, so a session that loses the port claim can distinguish a +// redundant same-owner collision from a different-owner one. +const ownerPath = "/owner" + // newCredentialsHandler returns an http.Handler that routes requests to the // appropriate handler function, which calls the CredentialsClient to get the // credentials and writes them to the response. // // Root is a readiness probe (see waitForServer); it must return 200 so the // server is detected as up. Unknown paths still 404 below. -func newCredentialsHandler(ctx context.Context, client CredentialsClient) http.Handler { +func newCredentialsHandler( + ctx context.Context, + client CredentialsClient, + owner string, +) http.Handler { routes := map[string]credentialsHandlerFunc{ "/": func(_ context.Context, writer http.ResponseWriter, _ *http.Request, _ CredentialsClient) error { writer.WriteHeader(http.StatusOK) return nil }, + ownerPath: func(_ context.Context, writer http.ResponseWriter, _ *http.Request, _ CredentialsClient) error { + writer.Header().Set("Content-Type", "text/plain; charset=utf-8") + writer.WriteHeader(http.StatusOK) + _, err := writer.Write([]byte(owner)) + return err + }, "/git-credentials": handleGitCredentialsRequest, "/docker-credentials": handleDockerCredentialsRequest, "/git-ssh-signature": handleGitSSHSignatureRequest, @@ -127,6 +151,41 @@ func newCredentialsHandler(ctx context.Context, client CredentialsClient) http.H }) } +// fetchOwnerTimeout bounds how long a session that lost the port claim +// waits to learn who currently owns it before giving up and treating the +// owner as unknown. +const fetchOwnerTimeout = 2 * time.Second + +// FetchOwner asks the credentials server already listening on port who it +// is serving. Returns an empty owner (with no error) if the server predates +// ownerPath or doesn't report one. +func FetchOwner(ctx context.Context, port int) (string, error) { + timeoutCtx, cancel := context.WithTimeout(ctx, fetchOwnerTimeout) + defer cancel() + + url := fmt.Sprintf("http://localhost:%d%s", port, ownerPath) + req, err := http.NewRequestWithContext(timeoutCtx, http.MethodGet, url, nil) + if err != nil { + return "", err + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return "", err + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode >= 400 { + return "", nil + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", err + } + return strings.TrimSpace(string(body)), nil +} + func GetPort() (int, error) { strPort := cmp.Or(os.Getenv(config.EnvCredentialsServerPort), DefaultPort) port, err := strconv.Atoi(strPort) diff --git a/pkg/credentials/server_test.go b/pkg/credentials/server_test.go index a5253430b..2cc41d513 100644 --- a/pkg/credentials/server_test.go +++ b/pkg/credentials/server_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "fmt" + "io" + "net" "net/http" "net/http/httptest" "strings" @@ -101,3 +103,49 @@ func TestHandleGitSSHSignature_GRPCSuccess_ReturnsJSON200(t *testing.T) { require.NoError(t, err) assert.Equal(t, "abc123", body["signature"]) } + +func TestOwnerEndpoint_ReturnsConfiguredOwner(t *testing.T) { + mock := &mockCredentialsClient{} + handler := newCredentialsHandler(context.Background(), mock, "alice") + + req := httptest.NewRequest(http.MethodGet, ownerPath, nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + resp := w.Result() + assert.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, "alice", string(body)) +} + +func TestFetchOwner_ReturnsConfiguredOwner(t *testing.T) { + ln, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + port := ln.Addr().(*net.TCPAddr).Port + + ctx := t.Context() + go func() { + _ = RunCredentialsServerWithListener(ctx, ln, &mockCredentialsClient{}, "bob") + }() + require.NoError(t, waitForServer(ctx, port)) + + owner, err := FetchOwner(context.Background(), port) + require.NoError(t, err) + assert.Equal(t, "bob", owner) +} + +func TestFetchOwner_EmptyWhenEndpointMissing(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + + var port int + _, err := fmt.Sscanf(server.URL, "http://127.0.0.1:%d", &port) + require.NoError(t, err) + + owner, err := FetchOwner(context.Background(), port) + require.NoError(t, err) + assert.Empty(t, owner) +} diff --git a/pkg/credentials/start.go b/pkg/credentials/start.go index db53a9e26..6177718f8 100644 --- a/pkg/credentials/start.go +++ b/pkg/credentials/start.go @@ -23,7 +23,7 @@ func StartCredentialsServer( } go func() { - err := RunCredentialsServer(ctx, port, client) + err := RunCredentialsServer(ctx, port, client, "") if err != nil { log.Errorf("error running git credentials server: error=%v", err) } From 1b1ec928c618caf5b344e8ff729884f15eb1a332 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 03:14:38 +0000 Subject: [PATCH 03/10] test(ssh): add e2e coverage for the credentials-server port race, trim comments Adds a real docker-backed e2e test: two concurrent devsy ssh sessions against the same freshly-created workspace must not surface credentials-server port errors in either session's stderr, matching the reported regression exactly (create workspace, connect via SSH, watch the terminal). Also trims comments across the two prior commits down to what's required to understand non-obvious rationale, and removes all comments from the Go test files touched by this change per project convention. --- .../agentcontainer/credentials_server.go | 16 +--- .../agentcontainer/credentials_server_test.go | 5 -- e2e/tests/ssh/credentials_server_race_test.go | 85 +++++++++++++++++++ pkg/credentials/server.go | 18 +--- pkg/credentials/server_test.go | 1 - 5 files changed, 93 insertions(+), 32 deletions(-) create mode 100644 e2e/tests/ssh/credentials_server_race_test.go diff --git a/cmd/internal/agentcontainer/credentials_server.go b/cmd/internal/agentcontainer/credentials_server.go index 267ae5b70..5c2abcb03 100644 --- a/cmd/internal/agentcontainer/credentials_server.go +++ b/cmd/internal/agentcontainer/credentials_server.go @@ -135,11 +135,8 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { return credentials.RunCredentialsServerWithListener(runCtx, ln, tunnelClient, cmd.User) } -// errPortOwnedByAnotherSession indicates the credentials-server port is -// already bound, almost certainly by another session's credentials-server -// for this same container. Only one session's credentials-server can hold -// this port at a time; losing the race is expected, not a failure, so -// callers should treat it as a no-op rather than an error. +// errPortOwnedByAnotherSession marks a bind failure as another session +// already owning the port, not a real error. var errPortOwnedByAnotherSession = errors.New("credentials server port owned by another session") // claimPort binds port and returns the listener, holding it exclusively so @@ -158,13 +155,8 @@ func claimPort(port int) (net.Listener, error) { return ln, nil } -// logPortOwnedByAnotherSession reports why this session is skipping -// credentials-server setup after losing the port claim. It fetches the -// owner of the server that won the race to tell apart the two outcomes: -// the same container user already has a working credentials server -// (redundant and harmless), versus a different user's session owns it, in -// which case cmd.User's own git/docker/signing helpers were never -// configured and won't work until that other session ends. +// logPortOwnedByAnotherSession warns only when the owning session is a +// different user, whose credential helpers were never configured. func (cmd *CredentialsServerCmd) logPortOwnedByAnotherSession(ctx context.Context, port int) { owner, err := credentials.FetchOwner(ctx, port) switch { diff --git a/cmd/internal/agentcontainer/credentials_server_test.go b/cmd/internal/agentcontainer/credentials_server_test.go index 6d755f024..8616f8173 100644 --- a/cmd/internal/agentcontainer/credentials_server_test.go +++ b/cmd/internal/agentcontainer/credentials_server_test.go @@ -89,9 +89,6 @@ func TestClaimPort_OnlyOneConcurrentCallerWins(t *testing.T) { } } -// startFakeCredentialsServer binds an ephemeral port and serves it as the -// given owner, simulating the session that wins the credentials-server port -// claim race. It returns the port once the server is dialable. func startFakeCredentialsServer(t *testing.T, owner string) int { t.Helper() @@ -151,8 +148,6 @@ func TestCredentialsServerCmd_Run_DifferentOwnerCollisionWarnsButDoesNotError(t assert.Contains(t, logged, "alice", "warning must name the owning session") } -// fakeCredentialsClient satisfies credentials.CredentialsClient for tests -// that only need a listening server, not real request handling. type fakeCredentialsClient struct{} func (fakeCredentialsClient) GitCredentials( diff --git a/e2e/tests/ssh/credentials_server_race_test.go b/e2e/tests/ssh/credentials_server_race_test.go new file mode 100644 index 000000000..22cd20dd1 --- /dev/null +++ b/e2e/tests/ssh/credentials_server_race_test.go @@ -0,0 +1,85 @@ +package ssh + +import ( + "context" + "os" + "sync" + "time" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +const ( + cmdWorkspace = "workspace" + cmdSSH = "ssh" +) + +var _ = ginkgo.Describe( + "devsy ssh credentials server race", + ginkgo.Label("ssh"), + ginkgo.Ordered, + func() { + var initialDir string + + ginkgo.BeforeEach(func() { + var err error + initialDir, err = os.Getwd() + framework.ExpectNoError(err) + }) + + ginkgo.It( + "should not surface credentials-server port errors when two ssh sessions race for the same workspace", + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(ctx context.Context) { + tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") + framework.ExpectNoError(err) + + f := framework.NewDefaultFramework(initialDir + "/bin") + _ = f.DevsyProviderAdd(ctx, "docker") + err = f.DevsyProviderUse(ctx, "docker") + framework.ExpectNoError(err) + + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) + framework.CleanupTempDir(initialDir, tempDir) + }) + + upDeadline := time.Now().Add(5 * time.Minute) + upCtx, cancelUp := context.WithDeadline(ctx, upDeadline) + defer cancelUp() + err = f.DevsyUp(upCtx, tempDir) + framework.ExpectNoError(err) + + const sessions = 2 + var wg sync.WaitGroup + stderrs := make([]string, sessions) + runErrs := make([]error, sessions) + for i := range sessions { + wg.Add(1) + go func(i int) { + defer wg.Done() + sshCtx, cancelSSH := context.WithDeadline( + ctx, + time.Now().Add(30*time.Second), + ) + defer cancelSSH() + _, stderr, sshErr := f.ExecCommandCapture(sshCtx, []string{ + cmdWorkspace, cmdSSH, tempDir, "--command", "sleep 2", + }) + stderrs[i] = stderr + runErrs[i] = sshErr + }(i) + } + wg.Wait() + + for i := range sessions { + framework.ExpectNoError(runErrs[i], "ssh session %d stderr: %s", i, stderrs[i]) + gomega.Expect(stderrs[i]).NotTo(gomega.ContainSubstring("not available")) + gomega.Expect(stderrs[i]).NotTo(gomega.ContainSubstring("credentials server")) + } + }, + ) + }, +) diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index 222de6921..5d4a17b85 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -59,11 +59,8 @@ func RunCredentialsServer( // exclusively (via net.Listen) from before startup through to serving, so no // other process can bind the same port in between. // -// owner identifies who this server is serving (e.g. the container user that -// configured it), exposed on ownerPath so a session that loses the port -// claim can tell whether skipping is safe (same owner: redundant, safe to -// skip) or not (different owner: that owner's credential helpers were never -// configured). +// owner is exposed on ownerPath so a losing session can tell a redundant +// same-owner collision from a different-owner one. func RunCredentialsServerWithListener( ctx context.Context, ln net.Listener, @@ -100,9 +97,7 @@ type credentialsHandlerFunc func( context.Context, http.ResponseWriter, *http.Request, CredentialsClient, ) error -// ownerPath serves the owner string RunCredentialsServerWithListener was -// started with, so a session that loses the port claim can distinguish a -// redundant same-owner collision from a different-owner one. +// ownerPath reports the owner a losing session's port claim collided with. const ownerPath = "/owner" // newCredentialsHandler returns an http.Handler that routes requests to the @@ -151,14 +146,9 @@ func newCredentialsHandler( }) } -// fetchOwnerTimeout bounds how long a session that lost the port claim -// waits to learn who currently owns it before giving up and treating the -// owner as unknown. const fetchOwnerTimeout = 2 * time.Second -// FetchOwner asks the credentials server already listening on port who it -// is serving. Returns an empty owner (with no error) if the server predates -// ownerPath or doesn't report one. +// FetchOwner returns "" without error if owner is unset or ownerPath is missing. func FetchOwner(ctx context.Context, port int) (string, error) { timeoutCtx, cancel := context.WithTimeout(ctx, fetchOwnerTimeout) defer cancel() diff --git a/pkg/credentials/server_test.go b/pkg/credentials/server_test.go index 2cc41d513..581694b98 100644 --- a/pkg/credentials/server_test.go +++ b/pkg/credentials/server_test.go @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/require" ) -// errReader is an io.Reader that always returns an error. type errReader struct{ err error } func (e *errReader) Read([]byte) (int, error) { return 0, e.err } From 9f8c110388cae0a987850ec5c3dc1dfd836535d1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 03:26:51 +0000 Subject: [PATCH 04/10] ci(e2e): one top-level label per e2e/tests/ssh file, matching CI matrix rows Each file in e2e/tests/ssh previously shared the 'ssh' label, with agent_forward.go and ssh.go additionally nesting 'agent-forward' and 'gpg' labels on individual specs, so a single CI matrix row ran every file's tests together and the secondary labels filtered nothing (no matching matrix entry existed for them). Give every file its own unique, top-level label and drop the nested per-spec labels: - ssh.go -> ssh - agent_forward.go -> agent-forward - ports_attributes_test.go -> ports-attributes - ssh_tunnel_mode_test.go -> ssh-tunnel-mode - credentials_server_race_test.go -> credentials-server-race No two files in the directory share a label, so no consolidation was needed. Split pr-ci.yml's single 'ssh' matrix row into five rows (one per label above) with the same settings as before, so each file's suite now runs as its own CI job. --- .github/workflows/pr-ci.yml | 24 +++++++++++++++++++ e2e/tests/ssh/agent_forward.go | 4 ---- e2e/tests/ssh/credentials_server_race_test.go | 2 +- e2e/tests/ssh/ports_attributes_test.go | 2 +- e2e/tests/ssh/ssh.go | 2 -- e2e/tests/ssh/ssh_tunnel_mode_test.go | 2 +- 6 files changed, 27 insertions(+), 9 deletions(-) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index b69972a12..ab152694f 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -275,6 +275,30 @@ jobs: install-kind: false requires-secret: false + - label: agent-forward + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + + - label: ports-attributes + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + + - label: ssh-tunnel-mode + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + + - label: credentials-server-race + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + - label: build runner: ubuntu-latest free-disk-space: false diff --git a/e2e/tests/ssh/agent_forward.go b/e2e/tests/ssh/agent_forward.go index 47d2c1958..fc34bdc01 100644 --- a/e2e/tests/ssh/agent_forward.go +++ b/e2e/tests/ssh/agent_forward.go @@ -21,7 +21,6 @@ import ( // per-connection socket directory must be cleaned up on disconnect. var _ = ginkgo.Describe( "devsy ssh agent forwarding", - ginkgo.Label("ssh"), ginkgo.Label("agent-forward"), ginkgo.Ordered, func() { @@ -253,8 +252,6 @@ var _ = ginkgo.Describe( ginkgo.It( "connection without any agent request still cleans up", - ginkgo.Label("ssh"), - ginkgo.Label("agent-forward"), ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx ginkgo.SpecContext) { tmpDir, err := os.MkdirTemp("", "devsy-ssh-cm-clean-") @@ -325,7 +322,6 @@ var _ = ginkgo.Describe( ginkgo.It( "parallel sessions on one connection observe the same socket concurrently", - ginkgo.Label("agent-forward"), ginkgo.SpecTimeout(framework.TimeoutModerate()), func(_ ginkgo.SpecContext) { controlPath, closeCM, err := framework.OpenSSHControlMaster( diff --git a/e2e/tests/ssh/credentials_server_race_test.go b/e2e/tests/ssh/credentials_server_race_test.go index 22cd20dd1..68baf9e3a 100644 --- a/e2e/tests/ssh/credentials_server_race_test.go +++ b/e2e/tests/ssh/credentials_server_race_test.go @@ -18,7 +18,7 @@ const ( var _ = ginkgo.Describe( "devsy ssh credentials server race", - ginkgo.Label("ssh"), + ginkgo.Label("credentials-server-race"), ginkgo.Ordered, func() { var initialDir string diff --git a/e2e/tests/ssh/ports_attributes_test.go b/e2e/tests/ssh/ports_attributes_test.go index df579385e..d32507138 100644 --- a/e2e/tests/ssh/ports_attributes_test.go +++ b/e2e/tests/ssh/ports_attributes_test.go @@ -17,7 +17,7 @@ import ( ) var _ = ginkgo.Describe("devsy portsAttributes e2e", - ginkgo.Label("ssh"), func() { + ginkgo.Label("ports-attributes"), func() { var initialDir string ginkgo.BeforeEach(func() { diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index ae081679e..c85da1067 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -74,7 +74,6 @@ var _ = ginkgo.Describe("devsy ssh test suite", ginkgo.Label("ssh"), ginkgo.Orde ginkgo.It( "should start workspace with GPG forwarding when host uses SSH signing format", - ginkgo.Label("gpg"), ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx ginkgo.SpecContext) { if runtime.GOOS == osWindows { @@ -124,7 +123,6 @@ var _ = ginkgo.Describe("devsy ssh test suite", ginkgo.Label("ssh"), ginkgo.Orde ginkgo.It( "should expose the host GPG secret key in the container via agent forwarding", - ginkgo.Label("gpg"), ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx ginkgo.SpecContext) { if runtime.GOOS == osWindows { diff --git a/e2e/tests/ssh/ssh_tunnel_mode_test.go b/e2e/tests/ssh/ssh_tunnel_mode_test.go index 09df6df4a..e86a43308 100644 --- a/e2e/tests/ssh/ssh_tunnel_mode_test.go +++ b/e2e/tests/ssh/ssh_tunnel_mode_test.go @@ -17,7 +17,7 @@ import ( var _ = ginkgo.Describe( "devsy ssh tunnel mode", - ginkgo.Label("ssh"), + ginkgo.Label("ssh-tunnel-mode"), ginkgo.Ordered, func() { var initialDir string From 2e3d4acb6b340956d56fb0e7b364dd92120e779f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 03:28:53 +0000 Subject: [PATCH 05/10] ci(e2e): prefix ssh sub-suite labels with ssh- to match directory name agent-forward, ports-attributes, and credentials-server-race become ssh-agent-forward, ssh-ports-attributes, and ssh-credentials-server-race in both the ginkgo.Label calls and the matching pr-ci.yml matrix rows. ssh and ssh-tunnel-mode already carried the prefix. --- .github/workflows/pr-ci.yml | 6 +++--- e2e/tests/ssh/agent_forward.go | 2 +- e2e/tests/ssh/credentials_server_race_test.go | 2 +- e2e/tests/ssh/ports_attributes_test.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index ab152694f..578232f87 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -275,13 +275,13 @@ jobs: install-kind: false requires-secret: false - - label: agent-forward + - label: ssh-agent-forward runner: ubuntu-latest free-disk-space: false install-kind: false requires-secret: false - - label: ports-attributes + - label: ssh-ports-attributes runner: ubuntu-latest free-disk-space: false install-kind: false @@ -293,7 +293,7 @@ jobs: install-kind: false requires-secret: false - - label: credentials-server-race + - label: ssh-credentials-server-race runner: ubuntu-latest free-disk-space: false install-kind: false diff --git a/e2e/tests/ssh/agent_forward.go b/e2e/tests/ssh/agent_forward.go index fc34bdc01..547390854 100644 --- a/e2e/tests/ssh/agent_forward.go +++ b/e2e/tests/ssh/agent_forward.go @@ -21,7 +21,7 @@ import ( // per-connection socket directory must be cleaned up on disconnect. var _ = ginkgo.Describe( "devsy ssh agent forwarding", - ginkgo.Label("agent-forward"), + ginkgo.Label("ssh-agent-forward"), ginkgo.Ordered, func() { var ( diff --git a/e2e/tests/ssh/credentials_server_race_test.go b/e2e/tests/ssh/credentials_server_race_test.go index 68baf9e3a..721b21aff 100644 --- a/e2e/tests/ssh/credentials_server_race_test.go +++ b/e2e/tests/ssh/credentials_server_race_test.go @@ -18,7 +18,7 @@ const ( var _ = ginkgo.Describe( "devsy ssh credentials server race", - ginkgo.Label("credentials-server-race"), + ginkgo.Label("ssh-credentials-server-race"), ginkgo.Ordered, func() { var initialDir string diff --git a/e2e/tests/ssh/ports_attributes_test.go b/e2e/tests/ssh/ports_attributes_test.go index d32507138..375bb0057 100644 --- a/e2e/tests/ssh/ports_attributes_test.go +++ b/e2e/tests/ssh/ports_attributes_test.go @@ -17,7 +17,7 @@ import ( ) var _ = ginkgo.Describe("devsy portsAttributes e2e", - ginkgo.Label("ports-attributes"), func() { + ginkgo.Label("ssh-ports-attributes"), func() { var initialDir string ginkgo.BeforeEach(func() { From 79bb6a32c86deaf5a87aaae2d2edf43bf04bc63a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 03:40:20 +0000 Subject: [PATCH 06/10] chore: drop two more comments per review Both explained rationale already conveyed by the surrounding code/naming. --- cmd/internal/agentcontainer/credentials_server.go | 2 -- pkg/credentials/server.go | 3 --- 2 files changed, 5 deletions(-) diff --git a/cmd/internal/agentcontainer/credentials_server.go b/cmd/internal/agentcontainer/credentials_server.go index 5c2abcb03..b69536114 100644 --- a/cmd/internal/agentcontainer/credentials_server.go +++ b/cmd/internal/agentcontainer/credentials_server.go @@ -155,8 +155,6 @@ func claimPort(port int) (net.Listener, error) { return ln, nil } -// logPortOwnedByAnotherSession warns only when the owning session is a -// different user, whose credential helpers were never configured. func (cmd *CredentialsServerCmd) logPortOwnedByAnotherSession(ctx context.Context, port int) { owner, err := credentials.FetchOwner(ctx, port) switch { diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index 5d4a17b85..094ef0fa2 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -58,9 +58,6 @@ func RunCredentialsServer( // already-bound listener. Use this when the caller must hold the port // exclusively (via net.Listen) from before startup through to serving, so no // other process can bind the same port in between. -// -// owner is exposed on ownerPath so a losing session can tell a redundant -// same-owner collision from a different-owner one. func RunCredentialsServerWithListener( ctx context.Context, ln net.Listener, From 223e8004d95b3d0ec8175dc067254df7d0bed634 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 04:18:54 +0000 Subject: [PATCH 07/10] fix(e2e): rename ssh sub-suite files off _test.go so ginkgo specs actually run go test -c ./e2e builds the top-level e2e package, which pulls in e2e/tests/ssh only as a regular blank-imported dependency; Go only compiles a package's *_test.go files when that package itself is under test, not when another package merely imports it. So ssh_tunnel_mode_test.go, ports_attributes_test.go, and credentials_server_race_test.go were silently excluded from the e2e binary the whole time - their specs never registered at all. This was invisible while all ssh/tests/ssh files shared the 'ssh' label, since ssh.go's own specs (which aren't _test.go-suffixed) kept that label's spec count above zero. Splitting into per-file labels surfaced it: --ginkgo.label-filter="ssh-tunnel-mode" (etc.) matched zero specs and failed --fail-on-empty. Renamed the three files to drop the _test.go suffix, matching the existing ssh.go/agent_forward.go convention. Verified via a locally built e2e.test binary that all five ssh labels now match a nonzero spec count (previously ssh-tunnel-mode/ssh-ports-attributes/ ssh-credentials-server-race matched 0). --- ...credentials_server_race_test.go => credentials_server_race.go} | 0 e2e/tests/ssh/{ports_attributes_test.go => ports_attributes.go} | 0 e2e/tests/ssh/{ssh_tunnel_mode_test.go => ssh_tunnel_mode.go} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename e2e/tests/ssh/{credentials_server_race_test.go => credentials_server_race.go} (100%) rename e2e/tests/ssh/{ports_attributes_test.go => ports_attributes.go} (100%) rename e2e/tests/ssh/{ssh_tunnel_mode_test.go => ssh_tunnel_mode.go} (100%) diff --git a/e2e/tests/ssh/credentials_server_race_test.go b/e2e/tests/ssh/credentials_server_race.go similarity index 100% rename from e2e/tests/ssh/credentials_server_race_test.go rename to e2e/tests/ssh/credentials_server_race.go diff --git a/e2e/tests/ssh/ports_attributes_test.go b/e2e/tests/ssh/ports_attributes.go similarity index 100% rename from e2e/tests/ssh/ports_attributes_test.go rename to e2e/tests/ssh/ports_attributes.go diff --git a/e2e/tests/ssh/ssh_tunnel_mode_test.go b/e2e/tests/ssh/ssh_tunnel_mode.go similarity index 100% rename from e2e/tests/ssh/ssh_tunnel_mode_test.go rename to e2e/tests/ssh/ssh_tunnel_mode.go From f21c325506e3c0051c62107b2bb9ccaa5080ec44 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 05:07:19 +0000 Subject: [PATCH 08/10] fix(e2e): background devsy up --ssh-tunnel instead of blocking on it ssh_tunnel_mode.go never actually ran before (it was _test.go-suffixed and thus excluded from the e2e binary, per the prior commit), so this bug in the test itself was never caught: devsy up --ssh-tunnel holds the CLI process open in the foreground until it receives a shutdown signal (cmd/workspace/up/up.go's finalizeUp blocks on <-ctx.Done() once a tunnel is active, by design - matching a long-running port-forward tool). The test called it through the framework's synchronous ExecCommandCapture and waited for it to return, so it just hung until the 5-minute spec timeout killed it. Added a small tunnelUpProcess helper that starts devsy up in the background, polls its combined output for the 'waiting for shutdown signal' line devsy already logs once the tunnel is active (config write and IDE launch happen before that point, so it's a safe readiness marker), then on cleanup sends SIGINT and waits for a clean exit (falling back to SIGKILL after 15s). Updated the four specs that pass --ssh-tunnel to use it; the fifth (ProxyCommand fallback, tunnel disabled) is unaffected and unchanged. Verified the process-management logic in isolation against a fake long-running script that mimics devsy up's exact behavior (prints the marker, blocks until SIGINT): waitUntilActive detects readiness and early-exit failures correctly, stop() shuts the process down cleanly via SIGINT well under the 15s force-kill fallback. Also fixed a pre-existing goconst violation in ports_attributes.go (bare "windows" literal instead of the existing osWindows const) surfaced by lint now that the file is no longer test-only. --- e2e/tests/ssh/ports_attributes.go | 6 +- e2e/tests/ssh/ssh_tunnel_mode.go | 144 +++++++++++++++++++++++++----- 2 files changed, 123 insertions(+), 27 deletions(-) diff --git a/e2e/tests/ssh/ports_attributes.go b/e2e/tests/ssh/ports_attributes.go index 375bb0057..2da537802 100644 --- a/e2e/tests/ssh/ports_attributes.go +++ b/e2e/tests/ssh/ports_attributes.go @@ -30,7 +30,7 @@ var _ = ginkgo.Describe("devsy portsAttributes e2e", "should forward port with onAutoForward=silent and skip port with onAutoForward=ignore", ginkgo.SpecTimeout(framework.TimeoutShort()), func(ctx context.Context) { - if runtime.GOOS == "windows" { + if runtime.GOOS == osWindows { ginkgo.Skip("skipping on windows") } @@ -132,7 +132,7 @@ var _ = ginkgo.Describe("devsy portsAttributes e2e", "should forward port with notify policy and apply label metadata", ginkgo.SpecTimeout(framework.TimeoutShort()), func(ctx context.Context) { - if runtime.GOOS == "windows" { + if runtime.GOOS == osWindows { ginkgo.Skip("skipping on windows") } @@ -209,7 +209,7 @@ var _ = ginkgo.Describe("devsy portsAttributes e2e", "should skip forwarding when requireLocalPort=true and host port is occupied", ginkgo.SpecTimeout(framework.TimeoutShort()), func(ctx context.Context) { - if runtime.GOOS == "windows" { + if runtime.GOOS == osWindows { ginkgo.Skip("skipping on windows") } diff --git a/e2e/tests/ssh/ssh_tunnel_mode.go b/e2e/tests/ssh/ssh_tunnel_mode.go index e86a43308..2d1f835b6 100644 --- a/e2e/tests/ssh/ssh_tunnel_mode.go +++ b/e2e/tests/ssh/ssh_tunnel_mode.go @@ -1,12 +1,17 @@ package ssh import ( + "bytes" "context" + "fmt" "net" "os" + "os/exec" "path/filepath" "runtime" "strings" + "sync" + "syscall" "time" "github.com/devsy-org/devsy/e2e/framework" @@ -15,6 +20,101 @@ import ( "github.com/onsi/gomega" ) +const tunnelActiveMarker = "waiting for shutdown signal" + +const tunnelActiveTimeout = 4 * time.Minute + +type safeBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *safeBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *safeBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +type tunnelUpProcess struct { + cmd *exec.Cmd + output *safeBuffer + exited chan struct{} + exitErr error +} + +func startTunnelUp( + f *framework.Framework, workspace string, extraArgs ...string, +) (*tunnelUpProcess, error) { + args := []string{ + cmdWorkspace, "up", + names.Flag(names.Debug), + names.Flag(names.IDE), "none", + names.Flag(names.SSHTunnel), + } + args = append(args, extraArgs...) + args = append(args, workspace) + + // #nosec G204 -- test binary with controlled arguments + cmd := exec.Command(filepath.Join(f.DevsyBinDir, f.DevsyBinName), args...) + out := &safeBuffer{} + cmd.Stdout = out + cmd.Stderr = out + if err := cmd.Start(); err != nil { + return nil, fmt.Errorf("start devsy up --ssh-tunnel: %w", err) + } + + p := &tunnelUpProcess{cmd: cmd, output: out, exited: make(chan struct{})} + go func() { + p.exitErr = cmd.Wait() + close(p.exited) + }() + return p, nil +} + +func (p *tunnelUpProcess) waitUntilActive() error { + deadline := time.Now().Add(tunnelActiveTimeout) + for { + if strings.Contains(p.output.String(), tunnelActiveMarker) { + return nil + } + select { + case <-p.exited: + return fmt.Errorf( + "devsy up exited before the tunnel became active: %v\noutput:\n%s", + p.exitErr, p.output.String(), + ) + case <-time.After(50 * time.Millisecond): + } + if time.Now().After(deadline) { + return fmt.Errorf( + "devsy up did not report an active tunnel within %s\noutput:\n%s", + tunnelActiveTimeout, p.output.String(), + ) + } + } +} + +func (p *tunnelUpProcess) stop() { + select { + case <-p.exited: + return + default: + } + _ = p.cmd.Process.Signal(syscall.SIGINT) + select { + case <-p.exited: + case <-time.After(15 * time.Second): + _ = p.cmd.Process.Kill() + <-p.exited + } +} + var _ = ginkgo.Describe( "devsy ssh tunnel mode", ginkgo.Label("ssh-tunnel-mode"), @@ -48,9 +148,11 @@ var _ = ginkgo.Describe( framework.CleanupTempDir(initialDir, tempDir) }) - devsyUpCtx, cancel := context.WithDeadline(ctx, time.Now().Add(5*time.Minute)) - defer cancel() - err = f.DevsyUp(devsyUpCtx, tempDir, names.Flag(names.SSHTunnel)) + proc, err := startTunnelUp(f, tempDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(proc.stop) + + err = proc.waitUntilActive() framework.ExpectNoError(err) devsySSHCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(20*time.Second)) @@ -83,15 +185,11 @@ var _ = ginkgo.Describe( framework.CleanupTempDir(initialDir, tempDir) }) - devsyUpCtx, cancel := context.WithDeadline(ctx, time.Now().Add(5*time.Minute)) - defer cancel() - err = f.DevsyUp( - devsyUpCtx, - tempDir, - names.Flag(names.SSHTunnel), - "--ssh-config", - sshConfigPath, - ) + proc, err := startTunnelUp(f, tempDir, "--ssh-config", sshConfigPath) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(proc.stop) + + err = proc.waitUntilActive() framework.ExpectNoError(err) configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) @@ -136,15 +234,11 @@ var _ = ginkgo.Describe( framework.CleanupTempDir(initialDir, tempDir) }) - devsyUpCtx, cancel := context.WithDeadline(ctx, time.Now().Add(5*time.Minute)) - defer cancel() - err = f.DevsyUp( - devsyUpCtx, - tempDir, - names.Flag(names.SSHTunnel), - "--ssh-config", - sshConfigPath, - ) + proc, err := startTunnelUp(f, tempDir, "--ssh-config", sshConfigPath) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(proc.stop) + + err = proc.waitUntilActive() framework.ExpectNoError(err) configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) @@ -190,9 +284,11 @@ var _ = ginkgo.Describe( framework.CleanupTempDir(initialDir, tempDir) }) - devsyUpCtx, cancel := context.WithDeadline(ctx, time.Now().Add(5*time.Minute)) - defer cancel() - err = f.DevsyUp(devsyUpCtx, tempDir, names.Flag(names.SSHTunnel)) + proc, err := startTunnelUp(f, tempDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(proc.stop) + + err = proc.waitUntilActive() framework.ExpectNoError(err) for i := range 3 { From 9c4ed564ac780ba07a93baaa0ad59bff5ac6399a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 05:14:31 +0000 Subject: [PATCH 09/10] refactor(e2e): use gomega.Eventually/StopTrying instead of a hand-rolled poll loop waitUntilActive reinvented gomega.Eventually with a manual for{select{time.After()}} loop. This codebase already has an established convention for polling a growing log/output buffer for a marker - gomega.Eventually(fn).WithTimeout(...).WithPolling(...). Should(gomega.ContainSubstring(...)) - used throughout e2e/tests (e.g. ide/browser_returns.go's getTunnelLogsFn). Switched to it, threading the spec's ctx via WithContext so the poll also stops on spec cancellation, and using gomega.StopTrying(...).Wrap(err) to fail fast (with the process's real exit error) instead of waiting out the full timeout when devsy up exits early. waitUntilActive now asserts directly via Eventually rather than returning an error for callers to pass through ExpectNoError, matching how Eventently is used as the assertion itself elsewhere in this suite. stop()'s SIGINT+bounded-kill logic is unchanged: it is plain process lifecycle cleanup, not a spec assertion, and already uses the more precise cmd.Wait()-driven signal this repo's other raw-process helpers (e2e/framework/ssh_agent.go) rely on rather than a Gomega poll. Verified via a standalone program exercising the exact refactored waitUntilActive against fake scripts: the success case detects readiness and shuts down cleanly, and the early-exit case triggers StopTrying and fails immediately with the process's real error instead of hanging until the timeout. --- e2e/tests/ssh/ssh_tunnel_mode.go | 40 +++++++++++--------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/e2e/tests/ssh/ssh_tunnel_mode.go b/e2e/tests/ssh/ssh_tunnel_mode.go index 2d1f835b6..7692a7b9f 100644 --- a/e2e/tests/ssh/ssh_tunnel_mode.go +++ b/e2e/tests/ssh/ssh_tunnel_mode.go @@ -77,27 +77,19 @@ func startTunnelUp( return p, nil } -func (p *tunnelUpProcess) waitUntilActive() error { - deadline := time.Now().Add(tunnelActiveTimeout) - for { - if strings.Contains(p.output.String(), tunnelActiveMarker) { - return nil - } +func (p *tunnelUpProcess) waitUntilActive(ctx context.Context) { + gomega.Eventually(func() (string, error) { + out := p.output.String() select { case <-p.exited: - return fmt.Errorf( - "devsy up exited before the tunnel became active: %v\noutput:\n%s", - p.exitErr, p.output.String(), - ) - case <-time.After(50 * time.Millisecond): - } - if time.Now().After(deadline) { - return fmt.Errorf( - "devsy up did not report an active tunnel within %s\noutput:\n%s", - tunnelActiveTimeout, p.output.String(), - ) + return out, gomega.StopTrying( + "devsy up exited before the tunnel became active", + ).Wrap(p.exitErr) + default: + return out, nil } - } + }).WithContext(ctx).WithTimeout(tunnelActiveTimeout).WithPolling(100 * time.Millisecond). + Should(gomega.ContainSubstring(tunnelActiveMarker)) } func (p *tunnelUpProcess) stop() { @@ -152,8 +144,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) ginkgo.DeferCleanup(proc.stop) - err = proc.waitUntilActive() - framework.ExpectNoError(err) + proc.waitUntilActive(ctx) devsySSHCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(20*time.Second)) defer cancelSSH() @@ -189,8 +180,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) ginkgo.DeferCleanup(proc.stop) - err = proc.waitUntilActive() - framework.ExpectNoError(err) + proc.waitUntilActive(ctx) configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) framework.ExpectNoError(err) @@ -238,8 +228,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) ginkgo.DeferCleanup(proc.stop) - err = proc.waitUntilActive() - framework.ExpectNoError(err) + proc.waitUntilActive(ctx) configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) framework.ExpectNoError(err) @@ -288,8 +277,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) ginkgo.DeferCleanup(proc.stop) - err = proc.waitUntilActive() - framework.ExpectNoError(err) + proc.waitUntilActive(ctx) for i := range 3 { sshCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(20*time.Second)) From 6463551bd27b7a443d968df4e2123849b5f66d82 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 19 Aug 2026 05:35:32 +0000 Subject: [PATCH 10/10] fix(credentials): harden FetchOwner against a hostile port squatter FetchOwner is only called after claimPort's own bind failed with EADDRINUSE, so whatever answers on that port isn't necessarily our own credentials server - it could be another local user's unrelated or malicious process squatting the port inside a shared devcontainer, which is exactly the cross-user scenario this owner-lookup exists to detect in the first place. Trusting that response unconditionally was wrong: - http.DefaultClient follows redirects (up to 10), so a squatter could redirect the probe to an arbitrary URL (e.g. a cloud metadata endpoint) and have some of that response reflected into devsy's log output via the cross-user Warn message. - io.ReadAll(resp.Body) had no size limit. - resp.StatusCode >= 400 treated any 2xx/3xx as success; once redirects are rejected, a bare 3xx would otherwise fall through and get parsed as an owner value. Use a client with CheckRedirect returning http.ErrUseLastResponse (never follows, returns the 3xx response itself), require exactly http.StatusOK, and cap the body read with io.LimitReader. /owner only ever legitimately returns 200 with a short plain-text body, so none of this changes behavior against a real devsy credentials-server. Added TestFetchOwner_DoesNotFollowRedirects and TestFetchOwner_CapsResponseSize. --- pkg/credentials/server.go | 20 ++++++++++++++++--- pkg/credentials/server_test.go | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index 094ef0fa2..600e7b350 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -145,6 +145,20 @@ func newCredentialsHandler( const fetchOwnerTimeout = 2 * time.Second +// maxOwnerResponseSize bounds how much of the /owner response FetchOwner +// reads. Port claimPort failed to bind, so whatever is listening there +// isn't necessarily our own credentials server; cap the read instead of +// trusting it to behave. +const maxOwnerResponseSize = 4096 + +// fetchOwnerClient never follows redirects: /owner always answers 200 with +// a plain-text body, so a redirect means the port isn't ours to trust. +var fetchOwnerClient = &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, +} + // FetchOwner returns "" without error if owner is unset or ownerPath is missing. func FetchOwner(ctx context.Context, port int) (string, error) { timeoutCtx, cancel := context.WithTimeout(ctx, fetchOwnerTimeout) @@ -156,17 +170,17 @@ func FetchOwner(ctx context.Context, port int) (string, error) { return "", err } - resp, err := http.DefaultClient.Do(req) + resp, err := fetchOwnerClient.Do(req) if err != nil { return "", err } defer func() { _ = resp.Body.Close() }() - if resp.StatusCode >= 400 { + if resp.StatusCode != http.StatusOK { return "", nil } - body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxOwnerResponseSize)) if err != nil { return "", err } diff --git a/pkg/credentials/server_test.go b/pkg/credentials/server_test.go index 581694b98..10d56dac4 100644 --- a/pkg/credentials/server_test.go +++ b/pkg/credentials/server_test.go @@ -148,3 +148,39 @@ func TestFetchOwner_EmptyWhenEndpointMissing(t *testing.T) { require.NoError(t, err) assert.Empty(t, owner) } + +func TestFetchOwner_DoesNotFollowRedirects(t *testing.T) { + evil := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("evil-owner")) + })) + defer evil.Close() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, evil.URL, http.StatusFound) + })) + defer server.Close() + + var port int + _, err := fmt.Sscanf(server.URL, "http://127.0.0.1:%d", &port) + require.NoError(t, err) + + owner, err := FetchOwner(context.Background(), port) + require.NoError(t, err) + assert.Empty(t, owner, "a redirect must not be followed to another owner value") +} + +func TestFetchOwner_CapsResponseSize(t *testing.T) { + oversized := strings.Repeat("a", maxOwnerResponseSize*2) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(oversized)) + })) + defer server.Close() + + var port int + _, err := fmt.Sscanf(server.URL, "http://127.0.0.1:%d", &port) + require.NoError(t, err) + + owner, err := FetchOwner(context.Background(), port) + require.NoError(t, err) + assert.LessOrEqual(t, len(owner), maxOwnerResponseSize) +}