Merge branch 'trunk' into niik/deploy

Author: niik

.github/workflows/lint.yml

@@ -46,7 +46,7 @@ jobs:
           exit $STATUS
 
       - name: golangci-lint
-        uses: golangci/golangci-lint-action@82606bf257cbaff209d206a39f5134f0cfbfd2ee # v9.2.1
+        uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
         with:
           version: v2.11.0
 

docs/install_linux.md

@@ -165,7 +165,7 @@ sudo zypper update gh
 
 [Homebrew](https://brew.sh/) is a free and open-source software package management system that simplifies the installation of software on Apple's operating system, macOS, as well as Linux.
 
-The [GitHub CLI formulae](https://formulae.brew.sh/formula/gh) is supported by the GitHub CLI maintainers with help from our friends at Homebrew with updates powered by [homebrew/hoomebrew-core](https://github.com/Homebrew/homebrew-core/blob/main/Formula/g/gh.rb).
+The [GitHub CLI formulae](https://formulae.brew.sh/formula/gh) is supported by the GitHub CLI maintainers with help from our friends at Homebrew with updates powered by [homebrew/homebrew-core](https://github.com/Homebrew/homebrew-core/blob/main/Formula/g/gh.rb).
 
 To install:
 
@@ -466,7 +466,7 @@ The [GitHub CLI package](https://webinstall.dev/gh/) is supported by the Webi co
 To install:
 
 ```shell
-curl -sS https://webi.sh/gh \| sh
+curl -sS https://webi.sh/gh | sh
 ```
 
 To upgrade:

docs/install_macos.md

@@ -6,7 +6,7 @@
 
 [Homebrew](https://brew.sh/) is a free and open-source software package management system that simplifies the installation of software on Apple's operating system, macOS, as well as Linux.
 
-The [GitHub CLI formulae](https://formulae.brew.sh/formula/gh) is supported by the GitHub CLI maintainers with help from our friends at Homebrew with updated powered by [homebrew/homebrew-core](https://github.com/Homebrew/homebrew-core/blob/main/Formula/g/gh.rb).
+The [GitHub CLI formulae](https://formulae.brew.sh/formula/gh) is supported by the GitHub CLI maintainers with help from our friends at Homebrew with updates powered by [homebrew/homebrew-core](https://github.com/Homebrew/homebrew-core/blob/main/Formula/g/gh.rb).
 
 To install:
 
@@ -113,7 +113,7 @@ The [GitHub CLI package](https://webinstall.dev/gh/) is supported by the Webi co
 To install:
 
 ```shell
-curl -sS https://webi.sh/gh \| sh
+curl -sS https://webi.sh/gh | sh
 ```
 
 To upgrade:

docs/install_windows.md

@@ -68,7 +68,7 @@ conda update gh --channel conda-forge
 
 ### Scoop
 
-The [GitHub CLI bucket](https://scoop.sh/#/apps?q=gh) is supported by the Scoop community with updated powered by [ScoopInstaller/Main](https://github.com/ScoopInstaller/Main/blob/master/bucket/gh.json).
+The [GitHub CLI bucket](https://scoop.sh/#/apps?q=gh) is supported by the Scoop community with updates powered by [ScoopInstaller/Main](https://github.com/ScoopInstaller/Main/blob/master/bucket/gh.json).
 
 To install:
 
@@ -91,7 +91,7 @@ The [GitHub CLI package](https://webinstall.dev/gh/) is supported by the Webi co
 To install:
 
 ```shell
-curl -sS https://webi.sh/gh \| sh
+curl -sS https://webi.sh/gh | sh
 ```
 
 To upgrade:

docs/primer/README.md

@@ -4,7 +4,7 @@ These guidelines are a collection of principles, foundations and usage guideline
 
 ## [Components](components)
 
-Design guidance on how we format content in in the Terminal through text formatting, color and font weights.
+Design guidance on how we format content in the Terminal through text formatting, color and font weights.
 
 ## [Foundations](foundations)
 

internal/codespaces/connection/connection.go

@@ -30,6 +30,10 @@ type CodespaceConnection struct {
 	Options                    *tunnels.TunnelRequestOptions
 	Tunnel                     *tunnels.Tunnel
 	AllowedPortPrivacySettings []string
+
+	// ManagerMu serializes access to TunnelManager operations which mutate
+	// shared state on the Tunnel object and are not goroutine-safe.
+	ManagerMu sync.Mutex
 }
 
 // NewCodespaceConnection initializes a connection to a codespace.

internal/codespaces/portforwarder/port_forwarder.go

@@ -36,7 +36,7 @@ type ForwardPortOpts struct {
 }
 
 type CodespacesPortForwarder struct {
-	connection      connection.CodespaceConnection
+	connection      *connection.CodespaceConnection
 	keepAliveReason chan string
 }
 
@@ -54,7 +54,7 @@ type PortForwarder interface {
 // NewPortForwarder returns a new PortForwarder for the specified codespace.
 func NewPortForwarder(ctx context.Context, codespaceConnection *connection.CodespaceConnection) (fwd PortForwarder, err error) {
 	return &CodespacesPortForwarder{
-		connection:      *codespaceConnection,
+		connection:      codespaceConnection,
 		keepAliveReason: make(chan string, 1),
 	}, nil
 }
@@ -108,6 +108,32 @@ func (fwd *CodespacesPortForwarder) ForwardPort(ctx context.Context, opts Forwar
 		return fmt.Errorf("error converting port: %w", err)
 	}
 
+	if err := fwd.createTunnelPort(ctx, port, opts); err != nil {
+		return err
+	}
+
+	// Connect to the tunnel
+	err = fwd.connection.Connect(ctx)
+	if err != nil {
+		return fmt.Errorf("connect failed: %v", err)
+	}
+
+	// Inform the host that we've forwarded the port locally
+	err = fwd.connection.TunnelClient.RefreshPorts(ctx)
+	if err != nil {
+		return fmt.Errorf("refresh ports failed: %v", err)
+	}
+
+	return nil
+}
+
+// createTunnelPort creates a tunnel port while holding the manager mutex.
+// TunnelManager operations mutate shared state on the Tunnel object and are
+// not goroutine-safe, so all calls are serialized under ManagerMu.
+func (fwd *CodespacesPortForwarder) createTunnelPort(ctx context.Context, port uint16, opts ForwardPortOpts) error {
+	fwd.connection.ManagerMu.Lock()
+	defer fwd.connection.ManagerMu.Unlock()
+
 	// In v0.0.25 of dev-tunnels, the dev-tunnel manager `CreateTunnelPort` would "accept" requests that
 	// change the port protocol but they would not result in any actual change. This has changed, resulting in
 	// an error `Invalid arguments. The tunnel port protocol cannot be changed.`. It's not clear why the previous
@@ -166,18 +192,6 @@ func (fwd *CodespacesPortForwarder) ForwardPort(ctx context.Context, opts Forwar
 		return fmt.Errorf("create tunnel port failed: %v", err)
 	}
 
-	// Connect to the tunnel
-	err = fwd.connection.Connect(ctx)
-	if err != nil {
-		return fmt.Errorf("connect failed: %v", err)
-	}
-
-	// Inform the host that we've forwarded the port locally
-	err = fwd.connection.TunnelClient.RefreshPorts(ctx)
-	if err != nil {
-		return fmt.Errorf("refresh ports failed: %v", err)
-	}
-
 	return nil
 }
 
@@ -243,6 +257,9 @@ func (fwd *CodespacesPortForwarder) ConnectToForwardedPort(ctx context.Context,
 
 // ListPorts fetches the list of ports that are currently forwarded.
 func (fwd *CodespacesPortForwarder) ListPorts(ctx context.Context) (ports []*tunnels.TunnelPort, err error) {
+	fwd.connection.ManagerMu.Lock()
+	defer fwd.connection.ManagerMu.Unlock()
+
 	ports, err = fwd.connection.TunnelManager.ListTunnelPorts(ctx, fwd.connection.Tunnel, fwd.connection.Options)
 	if err != nil {
 		return nil, fmt.Errorf("error listing ports: %w", err)
@@ -253,22 +270,27 @@ func (fwd *CodespacesPortForwarder) ListPorts(ctx context.Context) (ports []*tun
 
 // UpdatePortVisibility changes the visibility (private, org, public) of the specified port.
 func (fwd *CodespacesPortForwarder) UpdatePortVisibility(ctx context.Context, remotePort int, visibility string) error {
+	fwd.connection.ManagerMu.Lock()
 	tunnelPort, err := fwd.connection.TunnelManager.GetTunnelPort(ctx, fwd.connection.Tunnel, remotePort, fwd.connection.Options)
 	if err != nil {
+		fwd.connection.ManagerMu.Unlock()
 		return fmt.Errorf("error getting tunnel port: %w", err)
 	}
 
 	// If the port visibility isn't changing, don't do anything
 	if AccessControlEntriesToVisibility(tunnelPort.AccessControl.Entries) == visibility {
+		fwd.connection.ManagerMu.Unlock()
 		return nil
 	}
 
 	// Delete the existing tunnel port to update
 	port, err := convertIntToUint16(remotePort)
 	if err != nil {
+		fwd.connection.ManagerMu.Unlock()
 		return fmt.Errorf("error converting port: %w", err)
 	}
 	err = fwd.connection.TunnelManager.DeleteTunnelPort(ctx, fwd.connection.Tunnel, port, fwd.connection.Options)
+	fwd.connection.ManagerMu.Unlock()
 	if err != nil {
 		return fmt.Errorf("error deleting tunnel port: %w", err)
 	}

internal/codespaces/portforwarder/port_forwarder_test.go

@@ -7,6 +7,9 @@ import (
 	"github.com/cli/cli/v2/internal/codespaces/api"
 	"github.com/cli/cli/v2/internal/codespaces/connection"
 	"github.com/microsoft/dev-tunnels/go/tunnels"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sync/errgroup"
 )
 
 func TestNewPortForwarder(t *testing.T) {
@@ -31,26 +34,16 @@ func TestNewPortForwarder(t *testing.T) {
 
 	// Create the mock HTTP client
 	httpClient, err := connection.NewMockHttpClient()
-	if err != nil {
-		t.Fatalf("NewHttpClient returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	// Call the function being tested
 	conn, err := connection.NewCodespaceConnection(ctx, codespace, httpClient)
-	if err != nil {
-		t.Fatalf("NewCodespaceConnection returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	// Create the new port forwarder
 	portForwarder, err := NewPortForwarder(ctx, conn)
-	if err != nil {
-		t.Fatalf("NewPortForwarder returned an error: %v", err)
-	}
-
-	// Check that the port forwarder was created successfully
-	if portForwarder == nil {
-		t.Fatal("NewPortForwarder returned nil")
-	}
+	require.NoError(t, err)
+	require.NotNil(t, portForwarder)
 }
 
 func TestAccessControlEntriesToVisibility(t *testing.T) {
@@ -96,9 +89,7 @@ func TestAccessControlEntriesToVisibility(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			visibility := AccessControlEntriesToVisibility(test.accessControlEntries)
-			if visibility != test.expected {
-				t.Errorf("expected %q, got %q", test.expected, visibility)
-			}
+			assert.Equal(t, test.expected, visibility)
 		})
 	}
 }
@@ -131,9 +122,7 @@ func TestIsInternalPort(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			isInternal := IsInternalPort(test.port)
-			if isInternal != test.expected {
-				t.Errorf("expected %v, got %v", test.expected, isInternal)
-			}
+			assert.Equal(t, test.expected, isInternal)
 		})
 	}
 }
@@ -163,39 +152,70 @@ func TestForwardPortDefaultsToHTTPProtocol(t *testing.T) {
 	httpClient, err := connection.NewMockHttpClient(
 		connection.WithSpecificPorts(tunnelPorts),
 	)
-	if err != nil {
-		t.Fatalf("NewMockHttpClient returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	connection, err := connection.NewCodespaceConnection(t.Context(), codespace, httpClient)
-	if err != nil {
-		t.Fatalf("NewCodespaceConnection returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	fwd, err := NewPortForwarder(t.Context(), connection)
-	if err != nil {
-		t.Fatalf("NewPortForwarder returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	// When we forward a port without an existing one to use for a protocol, it should default to HTTP.
-	if err := fwd.ForwardPort(t.Context(), ForwardPortOpts{
+	err = fwd.ForwardPort(t.Context(), ForwardPortOpts{
 		Port: 1337,
-	}); err != nil {
-		t.Fatalf("ForwardPort returned an error: %v", err)
-	}
+	})
+	require.NoError(t, err)
 
 	ports, err := fwd.ListPorts(t.Context())
-	if err != nil {
-		t.Fatalf("ListPorts returned an error: %v", err)
-	}
+	require.NoError(t, err)
+	require.Len(t, ports, 1)
+	assert.Equal(t, string(tunnels.TunnelProtocolHttp), ports[0].Protocol)
+}
 
-	if len(ports) != 1 {
-		t.Fatalf("expected 1 port, got %d", len(ports))
+func TestConcurrentForwardPortDoesNotRace(t *testing.T) {
+	codespace := &api.Codespace{
+		Name:  "codespace-name",
+		State: api.CodespaceStateAvailable,
+		Connection: api.CodespaceConnection{
+			TunnelProperties: api.TunnelProperties{
+				ConnectAccessToken:     "tunnel access-token",
+				ManagePortsAccessToken: "manage-ports-token",
+				ServiceUri:             "http://global.rel.tunnels.api.visualstudio.com/",
+				TunnelId:               "tunnel-id",
+				ClusterId:              "usw2",
+				Domain:                 "domain.com",
+			},
+		},
+		RuntimeConstraints: api.RuntimeConstraints{
+			AllowedPortPrivacySettings: []string{"public", "private"},
+		},
 	}
 
-	if ports[0].Protocol != string(tunnels.TunnelProtocolHttp) {
-		t.Fatalf("expected port protocol to be http, got %s", ports[0].Protocol)
+	tunnelPorts := map[int]tunnels.TunnelPort{}
+
+	httpClient, err := connection.NewMockHttpClient(
+		connection.WithSpecificPorts(tunnelPorts),
+	)
+	require.NoError(t, err)
+
+	conn, err := connection.NewCodespaceConnection(t.Context(), codespace, httpClient)
+	require.NoError(t, err)
+
+	// Forward multiple ports concurrently from the same connection,
+	// mirroring what ForwardPorts does in ports.go.
+	group, ctx := errgroup.WithContext(t.Context())
+	for port := 3000; port < 3010; port++ {
+		fwd, err := NewPortForwarder(ctx, conn)
+		require.NoError(t, err)
+
+		group.Go(func() error {
+			return fwd.ForwardPort(ctx, ForwardPortOpts{
+				Port: port,
+			})
+		})
 	}
+
+	require.NoError(t, group.Wait())
 }
 
 func TestForwardPortRespectsProtocolOfExistingTunneledPorts(t *testing.T) {
@@ -230,38 +250,23 @@ func TestForwardPortRespectsProtocolOfExistingTunneledPorts(t *testing.T) {
 	httpClient, err := connection.NewMockHttpClient(
 		connection.WithSpecificPorts(tunnelPorts),
 	)
-	if err != nil {
-		t.Fatalf("NewMockHttpClient returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	connection, err := connection.NewCodespaceConnection(t.Context(), codespace, httpClient)
-	if err != nil {
-		t.Fatalf("NewCodespaceConnection returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	fwd, err := NewPortForwarder(t.Context(), connection)
-	if err != nil {
-		t.Fatalf("NewPortForwarder returned an error: %v", err)
-	}
+	require.NoError(t, err)
 
 	// When we forward a port, it would typically default to HTTP, to which the mock server would respond with a 400,
 	// but it should respect the existing port's protocol and forward it as HTTPS.
-	if err := fwd.ForwardPort(t.Context(), ForwardPortOpts{
+	err = fwd.ForwardPort(t.Context(), ForwardPortOpts{
 		Port: 1337,
-	}); err != nil {
-		t.Fatalf("ForwardPort returned an error: %v", err)
-	}
+	})
+	require.NoError(t, err)
 
 	ports, err := fwd.ListPorts(t.Context())
-	if err != nil {
-		t.Fatalf("ListPorts returned an error: %v", err)
-	}
-
-	if len(ports) != 1 {
-		t.Fatalf("expected 1 port, got %d", len(ports))
-	}
-
-	if ports[0].Protocol != string(tunnels.TunnelProtocolHttps) {
-		t.Fatalf("expected port protocol to be https, got %s", ports[0].Protocol)
-	}
+	require.NoError(t, err)
+	require.Len(t, ports, 1)
+	assert.Equal(t, string(tunnels.TunnelProtocolHttps), ports[0].Protocol)
 }