fix: prevent workspace.json race breaking VSCode browser create flow

cmd/root.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -20,8 +21,10 @@ import (
 	"github.com/devsy-org/devsy/cmd/use"
 	"github.com/devsy-org/devsy/pkg/config"
 	cliErrors "github.com/devsy-org/devsy/pkg/errors"
+	"github.com/devsy-org/devsy/pkg/exitcode"
 	"github.com/devsy-org/devsy/pkg/log"
 	"github.com/devsy-org/devsy/pkg/telemetry"
+	"github.com/devsy-org/devsy/pkg/workspace"
 	"github.com/go-logr/logr"
 	"github.com/spf13/cobra"
 	"golang.org/x/crypto/ssh"
@@ -78,6 +81,12 @@ func Execute() {
 				fmt.Fprintf(os.Stderr, "See:   %s\n", cliErr.DocURL)
 			}
 		}
+		// Signal workspace-not-found via a distinct exit code so parent
+		// processes (e.g. SetupBackhaul) can detect the registration race
+		// without parsing stderr.
+		if errors.Is(err, workspace.ErrWorkspaceNotFound) {
+			os.Exit(exitcode.WorkspaceNotFound)
+		}
 		os.Exit(1)
 	}
 }

cmd/ssh.go

@@ -421,6 +421,11 @@ func (cmd *SSHCmd) reverseForwardPorts(
 				mapping.Container.Address,
 				timeout,
 			)
+			if errors.Is(err, devssh.ErrIdleTimeout) {
+				log.Infof("reverse port-forward %s exited due to idle timeout", portMapping)
+				errChan <- nil
+				return
+			}
 			if !errors.Is(io.EOF, err) {
 				errChan <- fmt.Errorf("error forwarding %s: %w", portMapping, err)
 			}
@@ -464,6 +469,11 @@ func (cmd *SSHCmd) forwardPorts(
 				mapping.Container.Address,
 				timeout,
 			)
+			if errors.Is(err, devssh.ErrIdleTimeout) {
+				log.Infof("port-forward %s exited due to idle timeout", portMapping)
+				errChan <- nil
+				return
+			}
 			if !errors.Is(io.EOF, err) {
 				errChan <- fmt.Errorf("error forwarding %s: %w", portMapping, err)
 			}

pkg/agent/agent.go

@@ -402,7 +402,7 @@ func writeWorkspaceInfo(file string, workspaceInfo *provider2.AgentWorkspaceInfo
 	}
 
 	// write workspace config
-	err = os.WriteFile(file, encoded, 0o600)
+	err = provider2.WriteFileAtomic(file, encoded, 0o600)
 	if err != nil {
 		return fmt.Errorf("write workspace config file: %w", err)
 	}

pkg/exitcode/codes.go

@@ -0,0 +1,10 @@
+// Package exitcode defines process exit codes used as a contract between
+// devsy and any parent process that spawns it as a subprocess (e.g. the
+// backhaul SSH command). Keeping these in a leaf package avoids import
+// cycles between the packages that emit and consume them.
+package exitcode
+
+// WorkspaceNotFound is emitted when devsy exits because the requested
+// workspace could not be located. Parent processes may treat this as a
+// transient signal (e.g. during a workspace-registration race) and retry.
+const WorkspaceNotFound = 75

pkg/provider/atomic.go

@@ -0,0 +1,45 @@
+package provider
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+)
+
+// WriteFileAtomic writes data to path atomically by writing to a sibling
+// temp file then renaming. Readers either see the old content or the new
+// content, never a partially-written file.
+func WriteFileAtomic(path string, data []byte, perm os.FileMode) error {
+	dir := filepath.Dir(path)
+	tmp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp-*")
+	if err != nil {
+		return fmt.Errorf("create temp file: %w", err)
+	}
+	tmpName := tmp.Name()
+	cleanup := func() { _ = os.Remove(tmpName) }
+
+	if _, err := tmp.Write(data); err != nil {
+		_ = tmp.Close()
+		cleanup()
+		return fmt.Errorf("write temp file: %w", err)
+	}
+	if err := tmp.Chmod(perm); err != nil {
+		_ = tmp.Close()
+		cleanup()
+		return fmt.Errorf("chmod temp file: %w", err)
+	}
+	if err := tmp.Sync(); err != nil {
+		_ = tmp.Close()
+		cleanup()
+		return fmt.Errorf("sync temp file: %w", err)
+	}
+	if err := tmp.Close(); err != nil {
+		cleanup()
+		return fmt.Errorf("close temp file: %w", err)
+	}
+	if err := os.Rename(tmpName, path); err != nil {
+		cleanup()
+		return fmt.Errorf("rename temp file: %w", err)
+	}
+	return nil
+}

pkg/provider/atomic_test.go

@@ -0,0 +1,68 @@
+package provider
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+func TestWriteFileAtomic_ConcurrentReadersSeeNoPartialWrites(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "workspace.json")
+
+	// Seed with valid JSON so the first reads succeed.
+	if err := WriteFileAtomic(path, []byte(`{"n":0}`), 0o600); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	var stop atomic.Bool
+	var wg sync.WaitGroup
+	readerErr := make(chan error, 4)
+
+	wg.Go(func() { atomicWriter(t, &stop, path) })
+	for range 3 {
+		wg.Go(func() { atomicReader(&stop, path, readerErr) })
+	}
+
+	time.Sleep(200 * time.Millisecond)
+	stop.Store(true)
+	wg.Wait()
+	close(readerErr)
+
+	for err := range readerErr {
+		t.Fatalf("reader observed partial write: %v", err)
+	}
+}
+
+func atomicWriter(t *testing.T, stop *atomic.Bool, path string) {
+	t.Helper()
+	for i := 0; !stop.Load(); i++ {
+		payload, _ := json.Marshal(map[string]int{"n": i})
+		if err := WriteFileAtomic(path, payload, 0o600); err != nil {
+			t.Errorf("write: %v", err)
+			return
+		}
+	}
+}
+
+func atomicReader(stop *atomic.Bool, path string, out chan<- error) {
+	for !stop.Load() {
+		data, err := os.ReadFile(path) //nolint:gosec // test reads a path under t.TempDir
+		if err != nil {
+			if os.IsNotExist(err) {
+				continue
+			}
+			out <- err
+			return
+		}
+		var v map[string]int
+		if err := json.Unmarshal(data, &v); err != nil {
+			out <- err
+			return
+		}
+	}
+}

pkg/provider/dir.go

@@ -131,7 +131,7 @@ func SaveProviderConfig(context string, provider *ProviderConfig) error {
 	}
 
 	providerConfigFile := filepath.Join(providerDir, ProviderConfigFile)
-	err = os.WriteFile(providerConfigFile, providerDirBytes, 0o600)
+	err = WriteFileAtomic(providerConfigFile, providerDirBytes, 0o600)
 	if err != nil {
 		return err
 	}
@@ -157,7 +157,7 @@ func SaveProInstanceConfig(context string, proInstance *ProInstance) error {
 	}
 
 	proInstanceConfigFile := filepath.Join(providerDir, ProInstanceConfigFile)
-	err = os.WriteFile(proInstanceConfigFile, proInstanceBytes, 0o600)
+	err = WriteFileAtomic(proInstanceConfigFile, proInstanceBytes, 0o600)
 	if err != nil {
 		return err
 	}
@@ -183,7 +183,7 @@ func SaveWorkspaceResult(workspace *Workspace, result *config2.Result) error {
 	}
 
 	workspaceResultFile := filepath.Join(workspaceDir, WorkspaceResultFile)
-	err = os.WriteFile(workspaceResultFile, resultBytes, 0o600)
+	err = WriteFileAtomic(workspaceResultFile, resultBytes, 0o600)
 	if err != nil {
 		return err
 	}
@@ -210,7 +210,7 @@ func SaveWorkspaceConfig(workspace *Workspace) error {
 
 	workspaceConfigFile := filepath.Join(workspaceDir, WorkspaceConfigFile)
 	// #nosec G306 -- TODO Consider using a more secure permission setting and ownership if needed.
-	err = os.WriteFile(workspaceConfigFile, workspaceConfigBytes, 0o644)
+	err = WriteFileAtomic(workspaceConfigFile, workspaceConfigBytes, 0o644)
 	if err != nil {
 		return err
 	}
@@ -236,7 +236,7 @@ func SaveMachineConfig(machine *Machine) error {
 	}
 
 	machineConfigFile := filepath.Join(machineDir, MachineConfigFile)
-	err = os.WriteFile(machineConfigFile, machineConfigBytes, 0o600)
+	err = WriteFileAtomic(machineConfigFile, machineConfigBytes, 0o600)
 	if err != nil {
 		return err
 	}

pkg/ssh/forward.go

@@ -2,6 +2,7 @@ package ssh
 
 import (
 	"context"
+	"errors"
 	"io"
 	"net"
 	"sync"
@@ -11,6 +12,12 @@ import (
 	"golang.org/x/crypto/ssh"
 )
 
+// ErrIdleTimeout is returned by PortForward / ReversePortForward when the
+// forwarder shuts down because it stayed idle longer than the configured
+// EXIT_AFTER_TIMEOUT. Callers that want to treat idle-timeout as a clean exit
+// should check for this error with errors.Is.
+var ErrIdleTimeout = errors.New("port forward idle timeout")
+
 type ForwardingFunction func(
 	net.Conn,
 	*ssh.Client,
@@ -64,27 +71,39 @@ func portForwarding(
 	exitAfterTimeout time.Duration,
 	forwardFn ForwardingFunction,
 ) error {
+	// Derive a child context so the idle-timeout handler can signal shutdown
+	// with a typed cause (ErrIdleTimeout) without killing the whole process.
+	fwdCtx, cancel := context.WithCancelCause(ctx)
+	defer cancel(nil)
+
 	done := make(chan struct{})
 	defer close(done)
 
 	go func() {
 		select {
 		case <-done:
-		case <-ctx.Done():
+		case <-fwdCtx.Done():
 			_ = listener.Close()
 		}
 	}()
 
-	counter := newConnectionCounter(ctx, exitAfterTimeout, func() {
-		log.Fatal(
-			"Stopping devsy up, because it stayed idle for a while. " +
+	counter := newConnectionCounter(fwdCtx, exitAfterTimeout, func() {
+		log.Infof(
+			"Stopping port-forward on %s: idle for a while. "+
 				"You can disable this via 'devsy context set-options -o EXIT_AFTER_TIMEOUT=false'",
+			srcAddr,
 		)
+		cancel(ErrIdleTimeout)
 	}, srcAddr)
 	for {
 		// waiting for a new connection
 		connection, err := listener.Accept()
 		if err != nil {
+			// If shutdown was caused by the idle timeout, surface that
+			// typed error so callers can choose to treat it as a clean exit.
+			if cause := context.Cause(fwdCtx); errors.Is(cause, ErrIdleTimeout) {
+				return ErrIdleTimeout
+			}
 			return err
 		}
 

pkg/ssh/forward_test.go

@@ -0,0 +1,93 @@
+package ssh
+
+import (
+	"context"
+	"errors"
+	"net"
+	"testing"
+	"time"
+
+	"golang.org/x/crypto/ssh"
+)
+
+// noopForward is a ForwardingFunction that immediately closes the local
+// connection. This lets portForwarding exercise its idle/cancellation paths
+// without needing a real *ssh.Client.
+func noopForward(localConn net.Conn, _ *ssh.Client, _, _ string) {
+	_ = localConn.Close()
+}
+
+// TestPortForwarding_IdleTimeoutReturnsErrIdleTimeout verifies that when the
+// idle timeout fires after a connection has come and gone, portForwarding
+// returns ErrIdleTimeout.
+func TestPortForwarding_IdleTimeoutReturnsErrIdleTimeout(t *testing.T) {
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
+	defer cancel()
+
+	errCh := make(chan error, 1)
+	go func() {
+		errCh <- portForwarding(
+			ctx, nil, lis,
+			lis.Addr().String(), "tcp", "127.0.0.1:0",
+			15*time.Millisecond, noopForward,
+		)
+	}()
+
+	// Open and close a connection so the connection counter starts the idle
+	// timer when it returns to zero.
+	conn, err := net.Dial("tcp", lis.Addr().String())
+	if err != nil {
+		t.Fatalf("failed to dial listener: %v", err)
+	}
+	_ = conn.Close()
+
+	select {
+	case got := <-errCh:
+		if !errors.Is(got, ErrIdleTimeout) {
+			t.Fatalf("expected ErrIdleTimeout, got %v", got)
+		}
+	case <-time.After(200 * time.Millisecond):
+		t.Fatal("timed out waiting for portForwarding to return")
+	}
+}
+
+// TestPortForwarding_ParentCancelNotIdleTimeout verifies that when the parent
+// context is canceled, the returned error is NOT ErrIdleTimeout.
+func TestPortForwarding_ParentCancelNotIdleTimeout(t *testing.T) {
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+
+	errCh := make(chan error, 1)
+	go func() {
+		errCh <- portForwarding(
+			ctx, nil, lis,
+			lis.Addr().String(), "tcp", "127.0.0.1:0",
+			// Use a long idle timeout so it cannot fire during the test.
+			10*time.Second, noopForward,
+		)
+	}()
+
+	// Cancel before any idle timer could be relevant.
+	cancel()
+
+	select {
+	case got := <-errCh:
+		if errors.Is(got, ErrIdleTimeout) {
+			t.Fatalf("expected non-ErrIdleTimeout error on parent cancel, got %v", got)
+		}
+		if got == nil {
+			t.Fatal("expected a non-nil error on parent cancel")
+		}
+	case <-time.After(200 * time.Millisecond):
+		t.Fatal("timed out waiting for portForwarding to return")
+	}
+}