From d50fd63cbd6e437452e2d65c6ef2c8ab63c18c53 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:33:44 -0500 Subject: [PATCH 01/11] fix(agent): scope workspace content folder by UID When a workspace was deleted and recreated under a new UID, the bind-mount source path was reused verbatim. Docker Desktop's macOS file-sharing VM caches /host_mnt entries by inode, so the recycled inode under the same path surfaced as "bind source path does not exist" on the next docker run. Suffix the content folder with the workspace UID so each recreate uses a fresh path, and clear the persisted ContentFolder on UID change so the recompute always picks up the new value. Also log host environment and bind-source resolution before docker run so future cache-race failures are diagnosable from the log alone. --- cmd/internal/agentworkspace/up.go | 1 + pkg/agent/agent.go | 9 ++- pkg/agent/workspace.go | 7 +- pkg/driver/docker/diagnostics.go | 101 ++++++++++++++++++++++++++ pkg/driver/docker/diagnostics_test.go | 22 ++++++ pkg/driver/docker/docker.go | 3 + 6 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 pkg/driver/docker/diagnostics.go create mode 100644 pkg/driver/docker/diagnostics_test.go diff --git a/cmd/internal/agentworkspace/up.go b/cmd/internal/agentworkspace/up.go index 572328f5d..203567925 100644 --- a/cmd/internal/agentworkspace/up.go +++ b/cmd/internal/agentworkspace/up.go @@ -544,6 +544,7 @@ func prepareWorkspace(params prepareWorkspaceParams) error { params.workspaceInfo.Workspace.Source.LocalFolder != "" { params.workspaceInfo.ContentFolder = agent.GetAgentWorkspaceContentDir( params.workspaceInfo.Origin, + params.workspaceInfo.Workspace.UID, ) } diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 6f5263417..7f28a9872 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -298,6 +298,9 @@ func decodeWorkspaceInfoAndWrite( ) return false, nil, err } + + // Drop the old UID's path so the recompute below uses the new one. + workspaceInfo.ContentFolder = "" } } @@ -331,9 +334,11 @@ func decodeWorkspaceInfoAndWrite( } } - // set content folder if workspaceInfo.ContentFolder == "" { - workspaceInfo.ContentFolder = GetAgentWorkspaceContentDir(workspaceDir) + workspaceInfo.ContentFolder = GetAgentWorkspaceContentDir( + workspaceDir, + workspaceInfo.Workspace.UID, + ) log.Debugf( "set content folder to default location: contentFolder=%s", workspaceInfo.ContentFolder, diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index 630265792..d158237b7 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -215,8 +215,11 @@ echo Devsy return true, nil } -func GetAgentWorkspaceContentDir(workspaceDir string) string { - return filepath.Join(workspaceDir, "content") +// GetAgentWorkspaceContentDir returns the bind-mount source for a workspace. +// The UID suffix ensures a delete-then-recreate cycle never reuses the same +// host path, avoiding Docker Desktop's stale /host_mnt inode cache. +func GetAgentWorkspaceContentDir(workspaceDir, uid string) string { + return filepath.Join(workspaceDir, "content-"+uid) } func GetAgentBinariesDirFromWorkspaceDir(workspaceDir string) (string, error) { diff --git a/pkg/driver/docker/diagnostics.go b/pkg/driver/docker/diagnostics.go new file mode 100644 index 000000000..5dcc9d118 --- /dev/null +++ b/pkg/driver/docker/diagnostics.go @@ -0,0 +1,101 @@ +package docker + +import ( + "bytes" + "context" + "fmt" + "io" + "os" + "runtime" + "strings" + "sync" + "time" + + "github.com/devsy-org/devsy/pkg/docker" + "github.com/devsy-org/devsy/pkg/log" +) + +func extractBindSources(args []string) []string { + var srcs []string + for i, a := range args { + if spec, ok := mountSpec(a, args, i); ok { + srcs = appendBindSrc(srcs, spec) + } + } + return srcs +} + +func mountSpec(arg string, args []string, i int) (string, bool) { + if arg == "--mount" && i+1 < len(args) { + return args[i+1], true + } + if rest, ok := strings.CutPrefix(arg, "--mount="); ok { + return rest, true + } + return "", false +} + +func appendBindSrc(dst []string, spec string) []string { + if !strings.Contains(spec, "type=bind") { + return dst + } + for part := range strings.SplitSeq(spec, ",") { + if v, ok := strings.CutPrefix(part, "src="); ok { + return append(dst, v) + } + if v, ok := strings.CutPrefix(part, "source="); ok { + return append(dst, v) + } + } + return dst +} + +// logBindSources records host-side resolution of each bind source. A +// "host=exists" line followed by docker reporting the path missing is the +// fingerprint of a Docker Desktop file-share cache race. +func logBindSources(args []string) { + for _, src := range extractBindSources(args) { + _, err := os.Lstat(src) + log.Infof("docker bind: src=%s exists=%t", src, err == nil) + } +} + +var hostEnvOnce sync.Once + +func logHostEnvOnce(ctx context.Context, helper *docker.DockerHelper) { + hostEnvOnce.Do(func() { + log.Infof("docker host: %s", collectHostEnv(ctx, helper)) + }) +} + +func collectHostEnv(ctx context.Context, helper *docker.DockerHelper) string { + parts := []string{fmt.Sprintf("os=%s/%s", runtime.GOOS, runtime.GOARCH)} + if helper != nil { + if v := dockerInfoField(ctx, helper, "{{.ServerVersion}}"); v != "" { + parts = append(parts, "server="+v) + } + if v := dockerInfoField(ctx, helper, "{{.Driver}}"); v != "" { + parts = append(parts, "storageDriver="+v) + } + } + return strings.Join(parts, " ") +} + +func dockerInfoField(ctx context.Context, helper *docker.DockerHelper, tmpl string) string { + if ctx == nil { + ctx = context.Background() + } + tctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + var out bytes.Buffer + if err := helper.Run( + tctx, + []string{"info", "--format", tmpl}, + nil, + &out, + io.Discard, + ); err != nil { + return "" + } + return strings.TrimSpace(out.String()) +} diff --git a/pkg/driver/docker/diagnostics_test.go b/pkg/driver/docker/diagnostics_test.go new file mode 100644 index 000000000..c5a27e492 --- /dev/null +++ b/pkg/driver/docker/diagnostics_test.go @@ -0,0 +1,22 @@ +package docker + +import ( + "reflect" + "testing" +) + +func TestExtractBindSources(t *testing.T) { + args := []string{ + "run", "--sig-proxy=false", + "--mount", "type=bind,source=/a/b,target=/x,consistency=consistent", + "--mount", "type=bind,src=/c/d,dst=/y", + "--mount=type=bind,src=/e/f,dst=/z", + "--mount", "type=volume,source=myvol,target=/v", // not a bind, ignored + "alpine", + } + got := extractBindSources(args) + want := []string{"/a/b", "/c/d", "/e/f"} + if !reflect.DeepEqual(got, want) { + t.Errorf("extractBindSources() = %v, want %v", got, want) + } +} diff --git a/pkg/driver/docker/docker.go b/pkg/driver/docker/docker.go index 155433d1b..5526b2fe6 100644 --- a/pkg/driver/docker/docker.go +++ b/pkg/driver/docker/docker.go @@ -807,6 +807,9 @@ func (d *dockerDriver) startContainer( dir, ) + logHostEnvOnce(ctx, d.Docker) + logBindSources(args) + err := d.Docker.RunWithDir(ctx, dir, args, nil, writer, writer) if err != nil { log.Errorf( From ce6cd178ef2be74152ea475c3cd18362fe2d096d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:42:34 -0500 Subject: [PATCH 02/11] refactor(config): move container path constants to pkg/config/paths.go Container-side path constants (ContainerDataDir, ContainerDevsyHelperLocation, RemoteDevsyHelperLocation, ContainerActivityFile, WorkspaceBusyFile) were declared in pkg/agent next to runtime logic. They are pure paths derived from config.BinaryName, so they belong in pkg/config alongside DevContainerResultPath and SSHSignatureHelperPath. No behavior change; all call sites updated. --- cmd/internal/agentcontainer/daemon.go | 9 ++++----- cmd/internal/agentworkspace/logs.go | 3 ++- cmd/internal/agentworkspace/up.go | 2 +- cmd/internal/fleet_server.go | 4 ++-- cmd/internal/ssh_server.go | 10 +++++----- cmd/workspace/ssh.go | 5 ++--- pkg/agent/agent.go | 21 +++++---------------- pkg/agent/delivery/kubernetes.go | 4 ++-- pkg/agent/delivery/kubernetes_test.go | 4 ++-- pkg/agent/delivery/legacy_shell.go | 3 ++- pkg/agent/delivery/local_docker.go | 4 ++-- pkg/agent/delivery/remote_docker.go | 4 ++-- pkg/agent/inject.go | 4 ++-- pkg/agent/workspace.go | 2 +- pkg/config/paths.go | 15 +++++++++++++++ pkg/devcontainer/setup.go | 7 ++++--- pkg/devcontainer/setup/setup.go | 5 ++--- pkg/dotfiles/dotfiles.go | 3 +-- pkg/ide/jetbrains/generic.go | 3 +-- pkg/ide/rstudio/rstudio.go | 3 +-- pkg/options/resolve.go | 2 +- pkg/tunnel/services.go | 2 +- 22 files changed, 60 insertions(+), 59 deletions(-) diff --git a/cmd/internal/agentcontainer/daemon.go b/cmd/internal/agentcontainer/daemon.go index 04a420ef7..ac6d73212 100644 --- a/cmd/internal/agentcontainer/daemon.go +++ b/cmd/internal/agentcontainer/daemon.go @@ -13,7 +13,6 @@ import ( "syscall" "time" - "github.com/devsy-org/devsy/pkg/agent" config2 "github.com/devsy-org/devsy/pkg/config" agentd "github.com/devsy-org/devsy/pkg/daemon/agent" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -25,7 +24,7 @@ import ( ) const ( - RootDir = agent.ContainerDataDir + RootDir = config2.ContainerDataDir DaemonConfigPath = "/var/run/secrets/" + config2.BinaryName + "/daemon_config" ) @@ -68,13 +67,13 @@ func (cmd *DaemonCmd) Run(c *cobra.Command, args []string) error { } if timeoutDuration > 0 { if err := os.WriteFile( - agent.ContainerActivityFile, + config2.ContainerActivityFile, nil, 0o666, ); err != nil { // #nosec G306 return fmt.Errorf("failed to create activity file: %w", err) } - if err := os.Chmod(agent.ContainerActivityFile, 0o666); err != nil { // #nosec G302 + if err := os.Chmod(config2.ContainerActivityFile, 0o666); err != nil { // #nosec G302 return fmt.Errorf("failed to set activity file permissions: %w", err) } } @@ -197,7 +196,7 @@ func runTimeoutMonitor( case <-ctx.Done(): return nil case <-ticker.C: - stat, err := os.Stat(agent.ContainerActivityFile) + stat, err := os.Stat(config2.ContainerActivityFile) if err != nil { continue } diff --git a/cmd/internal/agentworkspace/logs.go b/cmd/internal/agentworkspace/logs.go index 1baa93036..2f20207c8 100644 --- a/cmd/internal/agentworkspace/logs.go +++ b/cmd/internal/agentworkspace/logs.go @@ -7,6 +7,7 @@ import ( "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer" "github.com/spf13/cobra" ) @@ -61,7 +62,7 @@ func (cmd *LogsCmd) Run(ctx context.Context) error { // create new runner runner, err := devcontainer.NewRunner( - agent.ContainerDevsyHelperLocation, + config.ContainerDevsyHelperLocation, agent.DefaultAgentDownloadURL(), workspaceInfo, ) diff --git a/cmd/internal/agentworkspace/up.go b/cmd/internal/agentworkspace/up.go index 203567925..280c48642 100644 --- a/cmd/internal/agentworkspace/up.go +++ b/cmd/internal/agentworkspace/up.go @@ -208,7 +208,7 @@ func CreateRunner( workspaceInfo *provider.AgentWorkspaceInfo, ) (devcontainer.Runner, error) { return devcontainer.NewRunner( - agent.ContainerDevsyHelperLocation, + config.ContainerDevsyHelperLocation, agent.DefaultAgentDownloadURL(), workspaceInfo, ) diff --git a/cmd/internal/fleet_server.go b/cmd/internal/fleet_server.go index d390928ef..bdea9cf64 100644 --- a/cmd/internal/fleet_server.go +++ b/cmd/internal/fleet_server.go @@ -8,7 +8,7 @@ import ( "time" "github.com/devsy-org/devsy/cmd/flags" - "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/config" "github.com/spf13/cobra" ) @@ -62,7 +62,7 @@ func (c *FleetServerCmd) Run(cmd *cobra.Command, _ []string) error { // if ouf last occurrence of notify if "Notify ID connected" // we have an active session, so let's keep alive if strings.Contains(connString[len(connString)-1][0], "is connected") { - file, _ := os.Create(agent.ContainerActivityFile) + file, _ := os.Create(config.ContainerActivityFile) _ = file.Close() } case <-cmd.Context().Done(): diff --git a/cmd/internal/ssh_server.go b/cmd/internal/ssh_server.go index 5483ad7f7..255adaa76 100644 --- a/cmd/internal/ssh_server.go +++ b/cmd/internal/ssh_server.go @@ -7,7 +7,7 @@ import ( "time" "github.com/devsy-org/devsy/cmd/flags" - "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/log" sshserver "github.com/devsy-org/devsy/pkg/ssh/server" "github.com/devsy-org/devsy/pkg/ssh/server/port" @@ -121,10 +121,10 @@ func (cmd *SSHServerCmd) Run(_ *cobra.Command, _ []string) error { if cmd.Stdio { if cmd.TrackActivity { go func() { - _, err = os.Stat(agent.ContainerActivityFile) + _, err = os.Stat(config.ContainerActivityFile) if err != nil { if err := os.WriteFile( - agent.ContainerActivityFile, + config.ContainerActivityFile, nil, 0o666, ); err != nil { // #nosec G306 @@ -132,7 +132,7 @@ func (cmd *SSHServerCmd) Run(_ *cobra.Command, _ []string) error { return } if err := os.Chmod( - agent.ContainerActivityFile, + config.ContainerActivityFile, 0o666, ); err != nil { // #nosec G302 fmt.Fprintf(os.Stderr, "error setting file permissions: %v\n", err) @@ -142,7 +142,7 @@ func (cmd *SSHServerCmd) Run(_ *cobra.Command, _ []string) error { for { time.Sleep(time.Second * 10) - file, _ := os.Create(agent.ContainerActivityFile) + file, _ := os.Create(config.ContainerActivityFile) _ = file.Close() } }() diff --git a/cmd/workspace/ssh.go b/cmd/workspace/ssh.go index 9f310a94b..227654841 100644 --- a/cmd/workspace/ssh.go +++ b/cmd/workspace/ssh.go @@ -16,7 +16,6 @@ import ( "github.com/devsy-org/devsy/cmd/completion" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/cmd/machine" - "github.com/devsy-org/devsy/pkg/agent" client2 "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/client/clientimplementation" "github.com/devsy-org/devsy/pkg/config" @@ -543,7 +542,7 @@ func (cmd *SSHCmd) startTunnel( log.Debugf("Run outer container tunnel") commandArgs := []string{ - agent.ContainerDevsyHelperLocation, + config.ContainerDevsyHelperLocation, "internal", "ssh-server", "--track-activity", @@ -705,7 +704,7 @@ func (cmd *SSHCmd) setupGPGAgent( // Now we forward the agent socket to the remote, and setup remote gpg to use it forwardAgent := []string{ - agent.ContainerDevsyHelperLocation, + config.ContainerDevsyHelperLocation, "internal", "agent", "workspace", diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 7f28a9872..eb6221d34 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -23,19 +23,8 @@ import ( const DefaultInactivityTimeout = time.Minute * 20 -// ContainerDataDir is the base directory for Devsy data inside containers. -const ContainerDataDir = "/var/" + config.BinaryName - -const ContainerDevsyHelperLocation = "/usr/local/bin/" + config.BinaryName - -const RemoteDevsyHelperLocation = "/tmp/" + config.BinaryName - -const ContainerActivityFile = "/tmp/" + config.BinaryName + ".activity" - var defaultAgentDownloadURL = config.GitHubReleasesURL + "/download/" -const WorkspaceBusyFile = "workspace.lock" - func DefaultAgentDownloadURL() string { devsyAgentURL := os.Getenv(config.EnvAgentURL) if devsyAgentURL != "" { @@ -374,7 +363,7 @@ func decodeWorkspaceInfoAndWrite( } func CreateWorkspaceBusyFile(folder string) { - filePath := filepath.Join(folder, WorkspaceBusyFile) + filePath := filepath.Join(folder, config.WorkspaceBusyFile) _, err := os.Stat(filePath) if err == nil { return @@ -384,13 +373,13 @@ func CreateWorkspaceBusyFile(folder string) { } func HasWorkspaceBusyFile(folder string) bool { - filePath := filepath.Join(folder, WorkspaceBusyFile) + filePath := filepath.Join(folder, config.WorkspaceBusyFile) _, err := os.Stat(filePath) return err == nil } func DeleteWorkspaceBusyFile(folder string) { - _ = os.Remove(filepath.Join(folder, WorkspaceBusyFile)) + _ = os.Remove(filepath.Join(folder, config.WorkspaceBusyFile)) } func writeWorkspaceInfo(file string, workspaceInfo *provider2.AgentWorkspaceInfo) error { @@ -495,7 +484,7 @@ func Tunnel( return exec(ctx, "root", command, stdin, stdout, stderr) }, IsLocal: false, - RemoteAgentPath: ContainerDevsyHelperLocation, + RemoteAgentPath: config.ContainerDevsyHelperLocation, DownloadURL: DefaultAgentDownloadURL(), PreferDownloadFromRemoteUrl: Bool(false), Timeout: timeout, @@ -505,7 +494,7 @@ func Tunnel( } // build command - command := fmt.Sprintf("'%s' internal ssh-server --stdio", ContainerDevsyHelperLocation) + command := fmt.Sprintf("'%s' internal ssh-server --stdio", config.ContainerDevsyHelperLocation) if log.DebugEnabled() { command += " --debug" } diff --git a/pkg/agent/delivery/kubernetes.go b/pkg/agent/delivery/kubernetes.go index 9744f92b8..e778bd7bb 100644 --- a/pkg/agent/delivery/kubernetes.go +++ b/pkg/agent/delivery/kubernetes.go @@ -4,7 +4,7 @@ import ( "context" "fmt" - "github.com/devsy-org/devsy/pkg/agent" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/inject" "github.com/devsy-org/devsy/pkg/log" ) @@ -37,7 +37,7 @@ func (d *KubernetesDelivery) DeliverPostStart(ctx context.Context, opts PostStar } defer func() { _ = binary.Close() }() - destPath := agent.ContainerDevsyHelperLocation + destPath := pkgconfig.ContainerDevsyHelperLocation script := fmt.Sprintf( `set -e; t=$(mktemp %s.XXXXXX); cat > "$t" && chmod 755 "$t" && mv "$t" %s || { rm -f "$t"; exit 1; }`, destPath, diff --git a/pkg/agent/delivery/kubernetes_test.go b/pkg/agent/delivery/kubernetes_test.go index cc6c9b56f..28e4152e4 100644 --- a/pkg/agent/delivery/kubernetes_test.go +++ b/pkg/agent/delivery/kubernetes_test.go @@ -8,7 +8,7 @@ import ( "strings" "testing" - "github.com/devsy-org/devsy/pkg/agent" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -68,7 +68,7 @@ func TestKubernetesDelivery_DeliverPostStart_WritesBinary(t *testing.T) { require.NoError(t, err) - destPath := agent.ContainerDevsyHelperLocation + destPath := pkgconfig.ContainerDevsyHelperLocation assert.Contains(t, capturedCmd, "cat >") assert.Contains(t, capturedCmd, destPath) assert.Contains(t, capturedCmd, "chmod 755") diff --git a/pkg/agent/delivery/legacy_shell.go b/pkg/agent/delivery/legacy_shell.go index edcacf4ab..0c1f3dbaa 100644 --- a/pkg/agent/delivery/legacy_shell.go +++ b/pkg/agent/delivery/legacy_shell.go @@ -6,6 +6,7 @@ import ( "io" "github.com/devsy-org/devsy/pkg/agent" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/inject" "github.com/devsy-org/devsy/pkg/log" ) @@ -38,7 +39,7 @@ func (d *LegacyShellDelivery) DeliverPostStart(ctx context.Context, opts PostSta Ctx: ctx, Exec: d.ExecFunc, IsLocal: false, - RemoteAgentPath: agent.ContainerDevsyHelperLocation, + RemoteAgentPath: pkgconfig.ContainerDevsyHelperLocation, DownloadURL: d.downloadURL(), PreferDownloadFromRemoteUrl: agent.Bool(false), } diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index fd59522a8..f22607fe3 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -10,7 +10,7 @@ import ( "path/filepath" "strings" - "github.com/devsy-org/devsy/pkg/agent" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/version" @@ -289,5 +289,5 @@ func (d *LocalDockerDelivery) dockerCommand() string { } func binaryName() string { - return agent.ContainerDevsyHelperLocation[len("/usr/local/bin/"):] + return pkgconfig.ContainerDevsyHelperLocation[len("/usr/local/bin/"):] } diff --git a/pkg/agent/delivery/remote_docker.go b/pkg/agent/delivery/remote_docker.go index b646bf068..ac47f7187 100644 --- a/pkg/agent/delivery/remote_docker.go +++ b/pkg/agent/delivery/remote_docker.go @@ -8,7 +8,7 @@ import ( "os/exec" "path" - "github.com/devsy-org/devsy/pkg/agent" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/log" ) @@ -39,7 +39,7 @@ func (d *RemoteDockerDelivery) DeliverPostStart(ctx context.Context, opts PostSt return fmt.Errorf("container ID is required for remote docker delivery") } - destPath := agent.ContainerDevsyHelperLocation + destPath := pkgconfig.ContainerDevsyHelperLocation if err := d.ensureDir(ctx, destPath); err != nil { return fmt.Errorf("ensure target directory: %w", err) diff --git a/pkg/agent/inject.go b/pkg/agent/inject.go index e6824fbe7..847601671 100644 --- a/pkg/agent/inject.go +++ b/pkg/agent/inject.go @@ -43,7 +43,7 @@ type InjectOptions struct { // IsLocal indicates if the injection target is the local machine. IsLocal bool // RemoteAgentPath is the path where the agent binary should be placed on the remote machine. - // Defaults to RemoteDevsyHelperLocation. + // Defaults to config.RemoteDevsyHelperLocation. RemoteAgentPath string // DownloadURL is the base URL to download the agent binary from. Defaults to DefaultAgentDownloadURL(). DownloadURL string @@ -93,7 +93,7 @@ func (o *InjectOptions) Validate() error { func (o *InjectOptions) applyPathDefaults() { if o.RemoteAgentPath == "" { - o.RemoteAgentPath = RemoteDevsyHelperLocation + o.RemoteAgentPath = config.RemoteDevsyHelperLocation } if o.Timeout == 0 { o.Timeout = waitForInstanceConnectionTimeout diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index d158237b7..b1b8b5aaf 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -32,7 +32,7 @@ var extraSearchLocations = []string{ "/home/devsy/" + config.ConfigDirName + "/agent", "/opt/devsy/agent", "/var/lib/devsy/agent", - ContainerDataDir + "/agent", + config.ContainerDataDir + "/agent", } // EnvAgentInContainer is set to "1" by every SSH command builder that diff --git a/pkg/config/paths.go b/pkg/config/paths.go index c94a4adfd..ac158f854 100644 --- a/pkg/config/paths.go +++ b/pkg/config/paths.go @@ -19,4 +19,19 @@ const ( // DaemonProcessName is the name used for the fallback background daemon process // PID file and lock file in os.TempDir(). DaemonProcessName = BinaryName + ".daemon" + + // ContainerDataDir is the base directory for Devsy data inside containers. + ContainerDataDir = "/var/" + BinaryName + + // ContainerDevsyHelperLocation is where the Devsy agent binary lives inside containers. + ContainerDevsyHelperLocation = "/usr/local/bin/" + BinaryName + + // RemoteDevsyHelperLocation is the staging path for the Devsy agent on remote hosts. + RemoteDevsyHelperLocation = "/tmp/" + BinaryName + + // ContainerActivityFile is touched by SSH/fleet servers to record container liveness. + ContainerActivityFile = "/tmp/" + BinaryName + ".activity" + + // WorkspaceBusyFile is the per-workspace lock file written under the workspace folder. + WorkspaceBusyFile = "workspace.lock" ) diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index f388514a9..6cae8b2cd 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -16,6 +16,7 @@ import ( "github.com/devsy-org/devsy/pkg/agent/delivery" "github.com/devsy-org/devsy/pkg/agent/tunnelserver" "github.com/devsy-org/devsy/pkg/compress" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/devcontainer/crane" "github.com/devsy-org/devsy/pkg/devcontainer/sshtunnel" @@ -147,7 +148,7 @@ func (r *runner) legacyInject(ctx context.Context, timeout time.Duration) error ) }, IsLocal: false, - RemoteAgentPath: agent.ContainerDevsyHelperLocation, + RemoteAgentPath: pkgconfig.ContainerDevsyHelperLocation, DownloadURL: agent.DefaultAgentDownloadURL(), PreferDownloadFromRemoteUrl: agent.Bool(false), Timeout: timeout, @@ -252,7 +253,7 @@ func (r *runner) compressWorkspaceConfig() (string, error) { func (r *runner) buildSetupCommand(compressed, workspaceConfigCompressed string) string { log.Infof("setting up container") args := []string{ - shellescape.Quote(agent.ContainerDevsyHelperLocation), + shellescape.Quote(pkgconfig.ContainerDevsyHelperLocation), "internal", "agent", "container", "setup", "--setup-info", shellescape.Quote(compressed), "--container-workspace-info", shellescape.Quote(workspaceConfigCompressed), @@ -375,7 +376,7 @@ func (r *runner) executeSetup( func (r *runner) buildSSHTunnelCommand() string { args := []string{ - shellescape.Quote(agent.ContainerDevsyHelperLocation), + shellescape.Quote(pkgconfig.ContainerDevsyHelperLocation), "internal", "ssh-server", "--stdio", } diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index 3ea0bb5cd..05d5c0021 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -15,7 +15,6 @@ import ( "strings" "github.com/devsy-org/api/pkg/devsy" - "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/agent/tunnel" "github.com/devsy-org/devsy/pkg/command" pkgconfig "github.com/devsy-org/devsy/pkg/config" @@ -410,7 +409,7 @@ func shouldSkipKubeConfig(tunnelClient tunnel.TunnelClient) bool { return true } - markerPath := filepath.Join(agent.ContainerDataDir, "setupKubeConfig.marker") + markerPath := filepath.Join(pkgconfig.ContainerDataDir, "setupKubeConfig.marker") info, err := os.Stat(markerPath) if err == nil { if info.Mode().Perm()&0o022 != 0 { @@ -492,7 +491,7 @@ func ensureKubeConfigMaps(config *clientcmdapi.Config) *clientcmdapi.Config { } func markerFileExists(markerName string, markerContent string) (bool, error) { - markerName = filepath.Join(agent.ContainerDataDir, markerName+".marker") + markerName = filepath.Join(pkgconfig.ContainerDataDir, markerName+".marker") t, err := os.ReadFile(markerName) if err != nil && !os.IsNotExist(err) { return false, err diff --git a/pkg/dotfiles/dotfiles.go b/pkg/dotfiles/dotfiles.go index 88bc6b0d1..40e7680c9 100644 --- a/pkg/dotfiles/dotfiles.go +++ b/pkg/dotfiles/dotfiles.go @@ -6,7 +6,6 @@ import ( "slices" "strings" - "github.com/devsy-org/devsy/pkg/agent" client2 "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/config" config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -170,7 +169,7 @@ func buildDotCmd(p buildDotCmdParams) (*exec.Cmd, error) { p.client.Workspace(), "--log-output=raw", "--command", - agent.ContainerDevsyHelperLocation+" "+strings.Join(agentArguments, " "), + config.ContainerDevsyHelperLocation+" "+strings.Join(agentArguments, " "), ) execPath, err := os.Executable() if err != nil { diff --git a/pkg/ide/jetbrains/generic.go b/pkg/ide/jetbrains/generic.go index 455704822..34b976125 100644 --- a/pkg/ide/jetbrains/generic.go +++ b/pkg/ide/jetbrains/generic.go @@ -11,7 +11,6 @@ import ( "path/filepath" "runtime" - "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/command" config2 "github.com/devsy-org/devsy/pkg/config" copy2 "github.com/devsy-org/devsy/pkg/copy" @@ -110,7 +109,7 @@ func (o *GenericJetBrainsServer) GetVolume() string { } func (o *GenericJetBrainsServer) getDownloadFolder() string { - return filepath.Join(agent.ContainerDataDir, o.options.ID) + return filepath.Join(config2.ContainerDataDir, o.options.ID) } func (o *GenericJetBrainsServer) Install(setupInfo *config.Result) error { diff --git a/pkg/ide/rstudio/rstudio.go b/pkg/ide/rstudio/rstudio.go index ae88a719e..b27614647 100644 --- a/pkg/ide/rstudio/rstudio.go +++ b/pkg/ide/rstudio/rstudio.go @@ -14,7 +14,6 @@ import ( "runtime" "strings" - "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/config" copypkg "github.com/devsy-org/devsy/pkg/copy" @@ -38,7 +37,7 @@ var Options = ide.Options{ const ( DefaultServerPort = 8787 - downloadFolder = agent.ContainerDataDir + "/rstudio-server" + downloadFolder = config.ContainerDataDir + "/rstudio-server" dataFolder = "/usr/local/share/devsy/rstudio-server/data" // rstudioConfigFolder is where RStudio expects configuration. rstudioConfigFolder = "/etc/rstudio" diff --git a/pkg/options/resolve.go b/pkg/options/resolve.go index c780e99e3..6ee3ba7c2 100644 --- a/pkg/options/resolve.go +++ b/pkg/options/resolve.go @@ -335,7 +335,7 @@ func resolveAgentPathAndURL( } } if agentConfig.Path == "" { - agentConfig.Path = agent.RemoteDevsyHelperLocation + agentConfig.Path = config.RemoteDevsyHelperLocation } agentConfig.DownloadURL = resolver.ResolveDefaultValue(agentConfig.DownloadURL, options) if agentConfig.DownloadURL == "" { diff --git a/pkg/tunnel/services.go b/pkg/tunnel/services.go index 48ceb27a9..afba69008 100644 --- a/pkg/tunnel/services.go +++ b/pkg/tunnel/services.go @@ -147,7 +147,7 @@ func buildCredentialsCommand(opts RunServicesOptions) string { command := fmt.Sprintf( "%s%s internal agent container credentials-server --user %s", agent.ContainerAgentEnvPrefix, - shellescape.Quote(agent.ContainerDevsyHelperLocation), + shellescape.Quote(config.ContainerDevsyHelperLocation), shellescape.Quote(opts.User), ) if opts.ConfigureGitCredentials { From 6480219c73359e6dd83421f18e3ef8c9867f80ee Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:43:49 -0500 Subject: [PATCH 03/11] refactor(agent): split decodeWorkspaceInfoAndWrite into phases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function had grown to ~175 lines with five distinct phases (decode, maybe-rerun-as-root, ensure dir, handle stale workspace, resolve content folder + write) interleaved with a per-step "log.Errorf … return err" pattern that doubled every failure in the log. Extract handleStaleWorkspace and resolveContentFolder so the orchestrator reads top-to-bottom in ~40 lines. Drop the redundant pre-return Errorf pairs — wrapped errors already carry the cause. Preserve the operationally meaningful INFO line ("delete old workspace: …") that ops greps on. No behavior change. --- pkg/agent/agent.go | 201 +++++++++++++++------------------------------ 1 file changed, 64 insertions(+), 137 deletions(-) diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index eb6221d34..679a92ed4 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -187,179 +187,106 @@ func decodeWorkspaceInfoAndWrite( writeInfo bool, deleteWorkspace func(workspaceInfo *provider2.AgentWorkspaceInfo) error, ) (bool, *provider2.AgentWorkspaceInfo, error) { - log.Debugf( - "starting to decode and write workspace info: workspaceEncodedLength=%v, writeInfo=%v", - len(workspaceInfoEncoded), - writeInfo, - ) - workspaceInfo, _, err := DecodeWorkspaceInfo(workspaceInfoEncoded) if err != nil { - log.Errorf( - "failed to decode workspace info: error=%v, workspaceEncodedLength=%v", - err, - len(workspaceInfoEncoded), - ) return false, nil, err } - log.Debugf( - "decoded workspace info: workspaceId=%s, context=%s, driver=%s", - workspaceInfo.Workspace.ID, - workspaceInfo.Workspace.Context, - workspaceInfo.Agent.Driver, - ) - - // check if we need to become root - log.Debug("checking if root privileges are required") - shouldExit, err := rerunAsRoot(workspaceInfo) - if err != nil { - log.Errorf("failed to rerun as root: error=%v", err) + if shouldExit, err := rerunAsRoot(workspaceInfo); err != nil { return false, nil, fmt.Errorf("rerun as root: %w", err) } else if shouldExit { - log.Debug("rerunning as root, exiting current process") return true, nil, nil } - // write to workspace folder - log.Debugf( - "creating agent workspace directory: dataPath=%s, context=%s, workspaceId=%s", - workspaceInfo.Agent.DataPath, - workspaceInfo.Workspace.Context, - workspaceInfo.Workspace.ID, - ) workspaceDir, err := CreateAgentWorkspaceDir( workspaceInfo.Agent.DataPath, workspaceInfo.Workspace.Context, workspaceInfo.Workspace.ID, ) if err != nil { - log.Errorf( - "failed to create agent workspace directory: error=%v, dataPath=%s, context=%s, workspaceId=%s", - err, - workspaceInfo.Agent.DataPath, - workspaceInfo.Workspace.Context, - workspaceInfo.Workspace.ID, - ) - return false, nil, err + return false, nil, fmt.Errorf("create workspace dir: %w", err) } - log.Debugf("using workspace dir: workspaceDir=%s", workspaceDir) - - // check if workspace config already exists workspaceConfig := filepath.Join(workspaceDir, provider2.WorkspaceConfigFile) if deleteWorkspace != nil { - log.Debugf("checking for existing workspace config: configFile=%s", workspaceConfig) - - oldWorkspaceInfo, _ := ParseAgentWorkspaceInfo(workspaceConfig) - if oldWorkspaceInfo != nil && - oldWorkspaceInfo.Workspace.UID != workspaceInfo.Workspace.UID { - // delete the old workspace - log.Infof( - "delete old workspace: workspaceId=%s, oldUid=%s, newUid=%s", - oldWorkspaceInfo.Workspace.ID, - oldWorkspaceInfo.Workspace.UID, - workspaceInfo.Workspace.UID, - ) - - err = deleteWorkspace(oldWorkspaceInfo) - if err != nil { - log.Errorf( - "failed to delete old workspace: error=%v, workspaceId=%s", - err, - oldWorkspaceInfo.Workspace.ID, - ) - return false, nil, fmt.Errorf("delete old workspace: %w", err) - } + workspaceDir, err = handleStaleWorkspace( + workspaceInfo, workspaceDir, workspaceConfig, deleteWorkspace, + ) + if err != nil { + return false, nil, err + } + } - // recreate workspace folder again - log.Debug("recreating workspace directory after deletion") - workspaceDir, err = CreateAgentWorkspaceDir( - workspaceInfo.Agent.DataPath, - workspaceInfo.Workspace.Context, - workspaceInfo.Workspace.ID, - ) - if err != nil { - log.Errorf( - "failed to recreate workspace directory: error=%v, dataPath=%s", - err, - workspaceInfo.Agent.DataPath, - ) - return false, nil, err - } + resolveContentFolder(workspaceInfo, workspaceDir) - // Drop the old UID's path so the recompute below uses the new one. - workspaceInfo.ContentFolder = "" + if writeInfo { + if err := writeWorkspaceInfo(workspaceConfig, workspaceInfo); err != nil { + return false, nil, fmt.Errorf("write workspace info: %w", err) } } - // check content folder for local folder workspace source - // - // We don't want to initialize the content folder with the value of the local workspace folder - // if we're running in proxy mode. - // We only have write access to /var/lib/loft/* by default causing nearly all local folders - // to run into permissions issues + workspaceInfo.Origin = workspaceDir + return false, workspaceInfo, nil +} + +// handleStaleWorkspace deletes a workspace whose persisted UID no longer +// matches the incoming one, then recreates the workspace dir. Returns the +// (possibly new) workspaceDir. +func handleStaleWorkspace( + workspaceInfo *provider2.AgentWorkspaceInfo, + workspaceDir, workspaceConfig string, + deleteWorkspace func(*provider2.AgentWorkspaceInfo) error, +) (string, error) { + oldWorkspaceInfo, _ := ParseAgentWorkspaceInfo(workspaceConfig) + if oldWorkspaceInfo == nil || + oldWorkspaceInfo.Workspace.UID == workspaceInfo.Workspace.UID { + return workspaceDir, nil + } + + log.Infof( + "delete old workspace: workspaceId=%s, oldUid=%s, newUid=%s", + oldWorkspaceInfo.Workspace.ID, + oldWorkspaceInfo.Workspace.UID, + workspaceInfo.Workspace.UID, + ) + if err := deleteWorkspace(oldWorkspaceInfo); err != nil { + return "", fmt.Errorf("delete old workspace: %w", err) + } + + newDir, err := CreateAgentWorkspaceDir( + workspaceInfo.Agent.DataPath, + workspaceInfo.Workspace.Context, + workspaceInfo.Workspace.ID, + ) + if err != nil { + return "", fmt.Errorf("recreate workspace dir: %w", err) + } + + // Drop the old UID's path so resolveContentFolder picks up the new one. + workspaceInfo.ContentFolder = "" + return newDir, nil +} + +// resolveContentFolder fills in workspaceInfo.ContentFolder, preferring a +// LocalFolder source when accessible. Platform proxy mode is excluded because +// it only has write access under /var/lib/loft/* and most local folders would +// hit permission errors. +func resolveContentFolder( + workspaceInfo *provider2.AgentWorkspaceInfo, + workspaceDir string, +) { if workspaceInfo.Workspace.Source.LocalFolder != "" && !workspaceInfo.CLIOptions.Platform.Enabled { - log.Debugf( - "checking local folder workspace source: localFolder=%s, workspaceOrigin=%s", - workspaceInfo.Workspace.Source.LocalFolder, - workspaceInfo.WorkspaceOrigin, - ) - - _, err = os.Stat(workspaceInfo.WorkspaceOrigin) - if err == nil { + if _, err := os.Stat(workspaceInfo.WorkspaceOrigin); err == nil { workspaceInfo.ContentFolder = workspaceInfo.Workspace.Source.LocalFolder - log.Debugf( - "set content folder to local folder: contentFolder=%s", - workspaceInfo.ContentFolder, - ) - } else { - log.Debugf( - "workspace origin not accessible: error=%v, workspaceOrigin=%s", - err, - workspaceInfo.WorkspaceOrigin, - ) } } - if workspaceInfo.ContentFolder == "" { workspaceInfo.ContentFolder = GetAgentWorkspaceContentDir( workspaceDir, workspaceInfo.Workspace.UID, ) - log.Debugf( - "set content folder to default location: contentFolder=%s", - workspaceInfo.ContentFolder, - ) - } - - // write workspace info - if writeInfo { - log.Debugf("writing workspace info to file: configFile=%s", workspaceConfig) - - err = writeWorkspaceInfo(workspaceConfig, workspaceInfo) - if err != nil { - log.Errorf( - "failed to write workspace info: error=%v, configFile=%s", - err, - workspaceConfig, - ) - return false, nil, err - } - - log.Debugf("wrote workspace info: configFile=%s", workspaceConfig) } - - workspaceInfo.Origin = workspaceDir - log.Debugf( - "processed workspace info: workspaceId=%s, origin=%s, contentFolder=%s", - workspaceInfo.Workspace.ID, - workspaceInfo.Origin, - workspaceInfo.ContentFolder, - ) - - return false, workspaceInfo, nil } func CreateWorkspaceBusyFile(folder string) { From 53db67d1d07931b4f0c2cd34b1dcca1e234ca4b4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:45:01 -0500 Subject: [PATCH 04/11] fix(docker): hoist "--mount" literal to a constant --- pkg/driver/docker/diagnostics.go | 4 +++- pkg/driver/docker/diagnostics_test.go | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/driver/docker/diagnostics.go b/pkg/driver/docker/diagnostics.go index 5dcc9d118..34231fc54 100644 --- a/pkg/driver/docker/diagnostics.go +++ b/pkg/driver/docker/diagnostics.go @@ -15,6 +15,8 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) +const mountFlag = "--mount" + func extractBindSources(args []string) []string { var srcs []string for i, a := range args { @@ -26,7 +28,7 @@ func extractBindSources(args []string) []string { } func mountSpec(arg string, args []string, i int) (string, bool) { - if arg == "--mount" && i+1 < len(args) { + if arg == mountFlag && i+1 < len(args) { return args[i+1], true } if rest, ok := strings.CutPrefix(arg, "--mount="); ok { diff --git a/pkg/driver/docker/diagnostics_test.go b/pkg/driver/docker/diagnostics_test.go index c5a27e492..42c150ca4 100644 --- a/pkg/driver/docker/diagnostics_test.go +++ b/pkg/driver/docker/diagnostics_test.go @@ -8,10 +8,10 @@ import ( func TestExtractBindSources(t *testing.T) { args := []string{ "run", "--sig-proxy=false", - "--mount", "type=bind,source=/a/b,target=/x,consistency=consistent", - "--mount", "type=bind,src=/c/d,dst=/y", + mountFlag, "type=bind,source=/a/b,target=/x,consistency=consistent", + mountFlag, "type=bind,src=/c/d,dst=/y", "--mount=type=bind,src=/e/f,dst=/z", - "--mount", "type=volume,source=myvol,target=/v", // not a bind, ignored + mountFlag, "type=volume,source=myvol,target=/v", // not a bind, ignored "alpine", } got := extractBindSources(args) From ed7ec7c49a145975dc0e68c38b089915205afa51 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:46:44 -0500 Subject: [PATCH 05/11] refactor(agent): bundle Tunnel parameters into TunnelOptions Tunnel had seven parameters; the four io.Reader/Writer values plus user and timeout fit naturally in an options struct. Keep ctx as the first positional arg per Go convention. --- cmd/internal/container_tunnel.go | 28 +++++++++++------------ pkg/agent/agent.go | 38 ++++++++++++++------------------ 2 files changed, 29 insertions(+), 37 deletions(-) diff --git a/cmd/internal/container_tunnel.go b/cmd/internal/container_tunnel.go index 20f5d8c63..ede6ecdfa 100644 --- a/cmd/internal/container_tunnel.go +++ b/cmd/internal/container_tunnel.go @@ -86,23 +86,21 @@ func (cmd *ContainerTunnelCmd) Run(ctx context.Context) error { os.Exit(0) }() - // create tunnel into container. - err = agent.Tunnel( - ctx, - func(ctx context.Context, user string, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { + return agent.Tunnel(ctx, agent.TunnelOptions{ + Exec: func( + ctx context.Context, + user, command string, + stdin io.Reader, + stdout, stderr io.Writer, + ) error { return runner.Command(ctx, user, command, stdin, stdout, stderr) }, - cmd.User, - os.Stdin, - os.Stdout, - os.Stderr, - workspaceInfo.InjectTimeout, - ) - if err != nil { - return err - } - - return nil + User: cmd.User, + Stdin: os.Stdin, + Stdout: os.Stdout, + Stderr: os.Stderr, + Timeout: workspaceInfo.InjectTimeout, + }) } func startDevContainer( diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 679a92ed4..ed7673da0 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -396,46 +396,40 @@ type Exec func( stdin io.Reader, stdout io.Writer, stderr io.Writer, ) error -func Tunnel( - ctx context.Context, - exec Exec, - user string, - stdin io.Reader, - stdout io.Writer, - stderr io.Writer, - timeout time.Duration, -) error { - err := InjectAgent(&InjectOptions{ +type TunnelOptions struct { + Exec Exec + User string + Stdin io.Reader + Stdout io.Writer + Stderr io.Writer + Timeout time.Duration +} + +func Tunnel(ctx context.Context, opts TunnelOptions) error { + if err := InjectAgent(&InjectOptions{ Ctx: ctx, Exec: func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { - return exec(ctx, "root", command, stdin, stdout, stderr) + return opts.Exec(ctx, "root", command, stdin, stdout, stderr) }, IsLocal: false, RemoteAgentPath: config.ContainerDevsyHelperLocation, DownloadURL: DefaultAgentDownloadURL(), PreferDownloadFromRemoteUrl: Bool(false), - Timeout: timeout, - }) - if err != nil { + Timeout: opts.Timeout, + }); err != nil { return err } - // build command command := fmt.Sprintf("'%s' internal ssh-server --stdio", config.ContainerDevsyHelperLocation) if log.DebugEnabled() { command += " --debug" } + user := opts.User if user == "" { user = "root" } - // create tunnel - err = exec(ctx, user, command, stdin, stdout, stderr) - if err != nil { - return err - } - - return nil + return opts.Exec(ctx, user, command, opts.Stdin, opts.Stdout, opts.Stderr) } func dockerReachable(dockerOverride string, envs map[string]string) (bool, error) { From 8fd008cbdb6a6c232c2d96ae375196bea7f1aa4e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:47:54 -0500 Subject: [PATCH 06/11] refactor(config): move agent download URL paths to config/repo.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lift AgentDownloadBaseURL and AgentLatestDownloadURL to pkg/config alongside GitHubReleasesURL — they're path constants, not runtime state. The DefaultAgentDownloadURL function stays in pkg/agent because it dispatches on env override and version (behavior, not a path). --- pkg/agent/agent.go | 13 ++++--------- pkg/config/repo.go | 6 ++++++ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index ed7673da0..bdb9a917d 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -23,19 +23,14 @@ import ( const DefaultInactivityTimeout = time.Minute * 20 -var defaultAgentDownloadURL = config.GitHubReleasesURL + "/download/" - func DefaultAgentDownloadURL() string { - devsyAgentURL := os.Getenv(config.EnvAgentURL) - if devsyAgentURL != "" { - return strings.TrimRight(devsyAgentURL, "/") + if override := os.Getenv(config.EnvAgentURL); override != "" { + return strings.TrimRight(override, "/") } - if version.GetVersion() == version.DevVersion { - return config.GitHubReleasesURL + "/latest/download" + return config.AgentLatestDownloadURL } - - return defaultAgentDownloadURL + version.GetVersion() + return config.AgentDownloadBaseURL + version.GetVersion() } func DecodeContainerWorkspaceInfo( diff --git a/pkg/config/repo.go b/pkg/config/repo.go index f91e0a377..9a8272c1d 100644 --- a/pkg/config/repo.go +++ b/pkg/config/repo.go @@ -23,4 +23,10 @@ const ( // WebsiteAssetsURL is the base URL for icon/image assets. WebsiteAssetsURL = WebsiteBaseURL + "/assets" + + // AgentDownloadBaseURL is the prefix under which versioned agent binaries are published. + AgentDownloadBaseURL = GitHubReleasesURL + "/download/" + + // AgentLatestDownloadURL points at the floating "latest" agent release. + AgentLatestDownloadURL = GitHubReleasesURL + "/latest/download" ) From d2b2322da8ff5c5b2c40deb9cf4e9bf093761e66 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:52:56 -0500 Subject: [PATCH 07/11] refactor(config): move DefaultAgentDownloadURL to config package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's a pure path resolver — picks between an env override, the latest URL, and the version-pinned URL — so it belongs alongside the URL constants in pkg/config/repo.go. The agent package no longer needs to import pkg/version. Tests move with the function. --- cmd/internal/agentworkspace/logs.go | 2 +- cmd/internal/agentworkspace/up.go | 2 +- pkg/agent/agent.go | 13 +------- pkg/agent/agent_test.go | 48 ----------------------------- pkg/agent/delivery/legacy_shell.go | 2 +- pkg/agent/inject.go | 6 ++-- pkg/config/repo.go | 20 ++++++++++++ pkg/config/repo_test.go | 44 ++++++++++++++++++++++++++ pkg/devcontainer/setup.go | 4 +-- pkg/options/resolve.go | 3 +- 10 files changed, 74 insertions(+), 70 deletions(-) delete mode 100644 pkg/agent/agent_test.go create mode 100644 pkg/config/repo_test.go diff --git a/cmd/internal/agentworkspace/logs.go b/cmd/internal/agentworkspace/logs.go index 2f20207c8..42bddb892 100644 --- a/cmd/internal/agentworkspace/logs.go +++ b/cmd/internal/agentworkspace/logs.go @@ -63,7 +63,7 @@ func (cmd *LogsCmd) Run(ctx context.Context) error { // create new runner runner, err := devcontainer.NewRunner( config.ContainerDevsyHelperLocation, - agent.DefaultAgentDownloadURL(), + config.DefaultAgentDownloadURL(), workspaceInfo, ) if err != nil { diff --git a/cmd/internal/agentworkspace/up.go b/cmd/internal/agentworkspace/up.go index 280c48642..28ed2a1b5 100644 --- a/cmd/internal/agentworkspace/up.go +++ b/cmd/internal/agentworkspace/up.go @@ -209,7 +209,7 @@ func CreateRunner( ) (devcontainer.Runner, error) { return devcontainer.NewRunner( config.ContainerDevsyHelperLocation, - agent.DefaultAgentDownloadURL(), + config.DefaultAgentDownloadURL(), workspaceInfo, ) } diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index bdb9a917d..3ce87bf98 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -18,21 +18,10 @@ import ( "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/log" provider2 "github.com/devsy-org/devsy/pkg/provider" - "github.com/devsy-org/devsy/pkg/version" ) const DefaultInactivityTimeout = time.Minute * 20 -func DefaultAgentDownloadURL() string { - if override := os.Getenv(config.EnvAgentURL); override != "" { - return strings.TrimRight(override, "/") - } - if version.GetVersion() == version.DevVersion { - return config.AgentLatestDownloadURL - } - return config.AgentDownloadBaseURL + version.GetVersion() -} - func DecodeContainerWorkspaceInfo( workspaceInfoRaw string, ) (*provider2.ContainerWorkspaceInfo, string, error) { @@ -408,7 +397,7 @@ func Tunnel(ctx context.Context, opts TunnelOptions) error { }, IsLocal: false, RemoteAgentPath: config.ContainerDevsyHelperLocation, - DownloadURL: DefaultAgentDownloadURL(), + DownloadURL: config.DefaultAgentDownloadURL(), PreferDownloadFromRemoteUrl: Bool(false), Timeout: opts.Timeout, }); err != nil { diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go deleted file mode 100644 index 8120e547f..000000000 --- a/pkg/agent/agent_test.go +++ /dev/null @@ -1,48 +0,0 @@ -package agent - -import ( - "os" - "testing" - - "github.com/devsy-org/devsy/pkg/config" - "github.com/stretchr/testify/suite" -) - -type AgentTestSuite struct { - suite.Suite - originalEnv string -} - -func (s *AgentTestSuite) SetupTest() { - s.originalEnv = os.Getenv(config.EnvAgentURL) -} - -func (s *AgentTestSuite) TearDownTest() { - if s.originalEnv != "" { - _ = os.Setenv(config.EnvAgentURL, s.originalEnv) - } else { - _ = os.Unsetenv(config.EnvAgentURL) - } -} - -func (s *AgentTestSuite) TestDefaultAgentDownloadURL_NoTrailingSlash() { - _ = os.Setenv(config.EnvAgentURL, "https://example.com/releases/latest/download") - result := DefaultAgentDownloadURL() - s.Equal("https://example.com/releases/latest/download", result) -} - -func (s *AgentTestSuite) TestDefaultAgentDownloadURL_SingleTrailingSlash() { - _ = os.Setenv(config.EnvAgentURL, "https://example.com/releases/latest/download/") - result := DefaultAgentDownloadURL() - s.Equal("https://example.com/releases/latest/download", result) -} - -func (s *AgentTestSuite) TestDefaultAgentDownloadURL_MultipleTrailingSlashes() { - _ = os.Setenv(config.EnvAgentURL, "https://example.com/releases/latest/download///") - result := DefaultAgentDownloadURL() - s.Equal("https://example.com/releases/latest/download", result) -} - -func TestAgentSuite(t *testing.T) { - suite.Run(t, new(AgentTestSuite)) -} diff --git a/pkg/agent/delivery/legacy_shell.go b/pkg/agent/delivery/legacy_shell.go index 0c1f3dbaa..7e498c30e 100644 --- a/pkg/agent/delivery/legacy_shell.go +++ b/pkg/agent/delivery/legacy_shell.go @@ -68,7 +68,7 @@ func (d *LegacyShellDelivery) downloadURL() string { if d.DownloadURL != "" { return d.DownloadURL } - return agent.DefaultAgentDownloadURL() + return pkgconfig.DefaultAgentDownloadURL() } // ExecFuncFromDriver creates an inject.ExecFunc that routes commands through diff --git a/pkg/agent/inject.go b/pkg/agent/inject.go index 847601671..3082d55ba 100644 --- a/pkg/agent/inject.go +++ b/pkg/agent/inject.go @@ -45,7 +45,7 @@ type InjectOptions struct { // RemoteAgentPath is the path where the agent binary should be placed on the remote machine. // Defaults to config.RemoteDevsyHelperLocation. RemoteAgentPath string - // DownloadURL is the base URL to download the agent binary from. Defaults to DefaultAgentDownloadURL(). + // DownloadURL is the base URL to download the agent binary from. Defaults to config.DefaultAgentDownloadURL(). DownloadURL string // PreferDownloadFromRemoteUrl forces downloading the agent even if a local binary is available. // Defaults to true for release versions, false for dev versions. @@ -108,7 +108,7 @@ func (o *InjectOptions) applyPathDefaults() { func (o *InjectOptions) applyURLDefaults() { if o.DownloadURL == "" { - o.DownloadURL = DefaultAgentDownloadURL() + o.DownloadURL = config.DefaultAgentDownloadURL() } if strings.Contains(o.DownloadURL, "github.com") && @@ -133,7 +133,7 @@ func (o *InjectOptions) applyPreferDownloadDefaults() { return } - isDefaultURL := o.DownloadURL == DefaultAgentDownloadURL() + isDefaultURL := o.DownloadURL == config.DefaultAgentDownloadURL() hasCustomAgentURL := os.Getenv(config.EnvAgentURL) != "" || !isDefaultURL preferDownloadEnv := os.Getenv(config.EnvAgentPreferDownload) diff --git a/pkg/config/repo.go b/pkg/config/repo.go index 9a8272c1d..4a821845f 100644 --- a/pkg/config/repo.go +++ b/pkg/config/repo.go @@ -1,5 +1,12 @@ package config +import ( + "os" + "strings" + + "github.com/devsy-org/devsy/pkg/version" +) + const ( RepoOwner = "devsy-org" RepoName = "devsy" @@ -30,3 +37,16 @@ const ( // AgentLatestDownloadURL points at the floating "latest" agent release. AgentLatestDownloadURL = GitHubReleasesURL + "/latest/download" ) + +// DefaultAgentDownloadURL returns the URL the host should download the agent +// binary from. Honors the DEVSY_AGENT_URL override; otherwise uses the +// version-pinned release URL, falling back to "latest" in dev builds. +func DefaultAgentDownloadURL() string { + if override := os.Getenv(EnvAgentURL); override != "" { + return strings.TrimRight(override, "/") + } + if version.GetVersion() == version.DevVersion { + return AgentLatestDownloadURL + } + return AgentDownloadBaseURL + version.GetVersion() +} diff --git a/pkg/config/repo_test.go b/pkg/config/repo_test.go new file mode 100644 index 000000000..d337c2d68 --- /dev/null +++ b/pkg/config/repo_test.go @@ -0,0 +1,44 @@ +package config + +import ( + "os" + "testing" + + "github.com/stretchr/testify/suite" +) + +type RepoTestSuite struct { + suite.Suite + originalEnv string +} + +func (s *RepoTestSuite) SetupTest() { + s.originalEnv = os.Getenv(EnvAgentURL) +} + +func (s *RepoTestSuite) TearDownTest() { + if s.originalEnv != "" { + _ = os.Setenv(EnvAgentURL, s.originalEnv) + } else { + _ = os.Unsetenv(EnvAgentURL) + } +} + +func (s *RepoTestSuite) TestDefaultAgentDownloadURL_NoTrailingSlash() { + _ = os.Setenv(EnvAgentURL, "https://example.com/releases/latest/download") + s.Equal("https://example.com/releases/latest/download", DefaultAgentDownloadURL()) +} + +func (s *RepoTestSuite) TestDefaultAgentDownloadURL_SingleTrailingSlash() { + _ = os.Setenv(EnvAgentURL, "https://example.com/releases/latest/download/") + s.Equal("https://example.com/releases/latest/download", DefaultAgentDownloadURL()) +} + +func (s *RepoTestSuite) TestDefaultAgentDownloadURL_MultipleTrailingSlashes() { + _ = os.Setenv(EnvAgentURL, "https://example.com/releases/latest/download///") + s.Equal("https://example.com/releases/latest/download", DefaultAgentDownloadURL()) +} + +func TestRepoSuite(t *testing.T) { + suite.Run(t, new(RepoTestSuite)) +} diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 6cae8b2cd..a6ec8bf74 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -124,7 +124,7 @@ func (r *runner) deliverPostStart(ctx context.Context, strategy delivery.AgentDe func (r *runner) newBinarySource() (delivery.BinarySourceFunc, error) { downloadURL := r.AgentDownloadURL if downloadURL == "" { - downloadURL = agent.DefaultAgentDownloadURL() + downloadURL = pkgconfig.DefaultAgentDownloadURL() } mgr, err := agent.NewBinaryManager(downloadURL) if err != nil { @@ -149,7 +149,7 @@ func (r *runner) legacyInject(ctx context.Context, timeout time.Duration) error }, IsLocal: false, RemoteAgentPath: pkgconfig.ContainerDevsyHelperLocation, - DownloadURL: agent.DefaultAgentDownloadURL(), + DownloadURL: pkgconfig.DefaultAgentDownloadURL(), PreferDownloadFromRemoteUrl: agent.Bool(false), Timeout: timeout, }) diff --git a/pkg/options/resolve.go b/pkg/options/resolve.go index 6ee3ba7c2..7aa58aa3c 100644 --- a/pkg/options/resolve.go +++ b/pkg/options/resolve.go @@ -8,7 +8,6 @@ import ( "reflect" "strings" - "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/options/resolver" "github.com/devsy-org/devsy/pkg/provider" @@ -383,7 +382,7 @@ func resolveAgentDownloadURL(devConfig *config.Config) string { return strings.TrimSuffix(contextAgentOption.Value, "/") + "/" } - return agent.DefaultAgentDownloadURL() + return config.DefaultAgentDownloadURL() } func filterResolvedOptions( From f9f0a825b19d94f47152571127ffede3c4552155 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 10:55:30 -0500 Subject: [PATCH 08/11] docs(agent): replace stale loft reference in resolveContentFolder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original comment referred to /var/lib/loft/* permissions from devsy's upstream fork. devsy doesn't use that path — the predicate is still correct (Platform-enabled runs can't address the host's LocalFolder), but the reasoning needed to match. --- pkg/agent/agent.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 3ce87bf98..85fda2fce 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -252,9 +252,8 @@ func handleStaleWorkspace( } // resolveContentFolder fills in workspaceInfo.ContentFolder, preferring a -// LocalFolder source when accessible. Platform proxy mode is excluded because -// it only has write access under /var/lib/loft/* and most local folders would -// hit permission errors. +// LocalFolder source when accessible. Skipped under Platform.Enabled because +// the host's local folder path is not addressable from the managed runner. func resolveContentFolder( workspaceInfo *provider2.AgentWorkspaceInfo, workspaceDir string, From d8665f67a7041510ca6b3b261b04e61261b915f8 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 11:22:51 -0500 Subject: [PATCH 09/11] refactor(ssh-server): rewrite cmd/internal ssh-server for clarity and safety MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Propagate context: Run takes ctx, the activity heartbeat exits on cancel instead of running until process exit. - Replace the os.Create/Close every-10s touch with os.Chtimes — same effect, no truncation, no nil-handle panic on failure. - Fix the "is in use" silent-success path: ListenAndServe now errors when the port is taken instead of logging and returning nil. - Wrap base64 decode errors instead of dropping them; uniform error formatting throughout. - Use errors.Is(err, fs.ErrNotExist) for the activity-file existence check so non-ENOENT stat errors surface. - Validate flag combinations up front: --track-activity now requires --stdio. - Extract parseSSHToken / decodeAuthorizedKeys / decodeBase64Bytes / ensureActivityFile / runActivityHeartbeat so each piece is independently testable; add unit tests covering the pure logic and ctx-cancel semantics. --- cmd/internal/ssh_server.go | 267 +++++++++++++++++++------------- cmd/internal/ssh_server_test.go | 140 +++++++++++++++++ 2 files changed, 303 insertions(+), 104 deletions(-) create mode 100644 cmd/internal/ssh_server_test.go diff --git a/cmd/internal/ssh_server.go b/cmd/internal/ssh_server.go index 255adaa76..c02cf6299 100644 --- a/cmd/internal/ssh_server.go +++ b/cmd/internal/ssh_server.go @@ -1,8 +1,11 @@ package cmdinternal import ( + "context" "encoding/base64" + "errors" "fmt" + "io/fs" "os" "time" @@ -17,6 +20,11 @@ import ( "github.com/spf13/cobra" ) +const ( + activityHeartbeatInterval = 10 * time.Second + activityFileMode = 0o666 +) + // SSHServerCmd holds the ssh server cmd flags. type SSHServerCmd struct { *flags.GlobalFlags @@ -30,138 +38,189 @@ type SSHServerCmd struct { } // NewSSHServerCmd creates a new ssh command. -func NewSSHServerCmd(flags *flags.GlobalFlags) *cobra.Command { - cmd := &SSHServerCmd{ - GlobalFlags: flags, - } +func NewSSHServerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { + cmd := &SSHServerCmd{GlobalFlags: globalFlags} sshCmd := &cobra.Command{ Use: "ssh-server", Short: "Starts a new SSH server", Args: cobra.NoArgs, - RunE: cmd.Run, - } - - sshCmd.Flags(). - StringVar(&cmd.Address, "address", fmt.Sprintf("0.0.0.0:%d", sshserver.DefaultPort), "Address to listen to") - sshCmd.Flags(). - BoolVar(&cmd.Stdio, "stdio", false, "Will listen on stdout and stdin instead of an address") - sshCmd.Flags(). - BoolVar(&cmd.TrackActivity, "track-activity", false, "If enabled will write the last activity time to a file") - sshCmd.Flags(). - StringVar(&cmd.ReuseSSHAuthSock, "reuse-ssh-auth-sock", "", - "If set, the SSH_AUTH_SOCK is expected to already be available in the workspace "+ - "(under /tmp using the key provided) and the connection reuses this instead of creating a new one") - _ = sshCmd.Flags().MarkHidden("reuse-ssh-auth-sock") - sshCmd.Flags().StringVar(&cmd.Token, "token", "", "Base64 encoded token to use") - sshCmd.Flags(). - StringVar(&cmd.Workdir, "workdir", "", "Directory where commands will run on the host") + RunE: func(cobraCmd *cobra.Command, _ []string) error { + return cmd.Run(cobraCmd.Context()) + }, + } + + f := sshCmd.Flags() + f.StringVar(&cmd.Address, "address", + fmt.Sprintf("0.0.0.0:%d", sshserver.DefaultPort), + "Address to listen to") + f.BoolVar(&cmd.Stdio, "stdio", false, + "Listen on stdin/stdout instead of an address") + f.BoolVar(&cmd.TrackActivity, "track-activity", false, + "Touch the activity file every "+activityHeartbeatInterval.String()+" (only with --stdio)") + f.StringVar(&cmd.ReuseSSHAuthSock, "reuse-ssh-auth-sock", "", + "If set, reuse a pre-existing SSH_AUTH_SOCK in the workspace under /tmp") + _ = f.MarkHidden("reuse-ssh-auth-sock") + f.StringVar(&cmd.Token, "token", "", "Base64 encoded token to use") + f.StringVar(&cmd.Workdir, "workdir", "", + "Directory where commands will run on the host") return sshCmd } // Run runs the command logic. -func (cmd *SSHServerCmd) Run(_ *cobra.Command, _ []string) error { - var ( - keys []ssh.PublicKey - hostKey []byte - err error - ) - if cmd.Token != "" { - // parse token - t, err := token.ParseToken(cmd.Token) - if err != nil { - return fmt.Errorf("parse token: %w", err) - } - - if t.AuthorizedKeys != "" { - keyBytes, err := base64.StdEncoding.DecodeString(t.AuthorizedKeys) - if err != nil { - return fmt.Errorf("seems like the provided encoded string is not base64 encoded") - } - - for len(keyBytes) > 0 { - key, _, _, rest, err := ssh.ParseAuthorizedKey(keyBytes) - if err != nil { - return fmt.Errorf("parse authorized key: %w", err) - } - - keys = append(keys, key) - keyBytes = rest - } - } +func (cmd *SSHServerCmd) Run(ctx context.Context) error { + if cmd.TrackActivity && !cmd.Stdio { + return errors.New("--track-activity requires --stdio") + } - if len(t.HostKey) > 0 { - hostKey, err = base64.StdEncoding.DecodeString(t.HostKey) - if err != nil { - return fmt.Errorf("decode host key") - } - } + keys, hostKey, err := parseSSHToken(cmd.Token) + if err != nil { + return err } // Sweep stale per-connection agent socket directories left behind by - // predecessors that - // were killed by docker exec / proxy-chain teardown before any internal - // SSH cleanup could run. Liveness is decided via a per-directory flock - // the owning process holds for its lifetime; the kernel releases the - // flock on any process exit (including SIGKILL). + // predecessors killed by docker exec / proxy-chain teardown before + // internal SSH cleanup could run. Liveness is decided via a per-directory + // flock the owning process holds for its lifetime; the kernel releases + // the flock on any process exit (including SIGKILL). sshserver.SweepStaleAgentSockets() - // start the server server, err := sshserver.NewServer( - cmd.Address, - hostKey, - keys, - cmd.Workdir, - cmd.ReuseSSHAuthSock, + cmd.Address, hostKey, keys, cmd.Workdir, cmd.ReuseSSHAuthSock, ) if err != nil { - return err + return fmt.Errorf("create ssh server: %w", err) } - // should we listen on stdout & stdin? if cmd.Stdio { - if cmd.TrackActivity { - go func() { - _, err = os.Stat(config.ContainerActivityFile) - if err != nil { - if err := os.WriteFile( - config.ContainerActivityFile, - nil, - 0o666, - ); err != nil { // #nosec G306 - fmt.Fprintf(os.Stderr, "error writing file: %v\n", err) - return - } - if err := os.Chmod( - config.ContainerActivityFile, - 0o666, - ); err != nil { // #nosec G302 - fmt.Fprintf(os.Stderr, "error setting file permissions: %v\n", err) - return - } - } - - for { - time.Sleep(time.Second * 10) - file, _ := os.Create(config.ContainerActivityFile) - _ = file.Close() - } - }() - } + return cmd.serveStdio(ctx, server) + } + return cmd.serveListener(server) +} - lis := stdio.NewStdioListener(os.Stdin, os.Stdout, true) - return server.Serve(lis) +func (cmd *SSHServerCmd) serveStdio(ctx context.Context, server sshserver.Server) error { + if cmd.TrackActivity { + go runActivityHeartbeat(ctx, config.ContainerActivityFile) } + lis := stdio.NewStdioListener(os.Stdin, os.Stdout, true) + return server.Serve(lis) +} - // check if ssh is already running at that port +func (cmd *SSHServerCmd) serveListener(server sshserver.Server) error { available, err := port.IsAvailable(cmd.Address) + if err != nil { + return fmt.Errorf("check port %s: %w", cmd.Address, err) + } if !available { + return fmt.Errorf("address %s already in use", cmd.Address) + } + return server.ListenAndServe() +} + +// parseSSHToken decodes the optional base64-encoded token blob. An empty +// string is valid and yields zero values: callers may run without a token. +func parseSSHToken(encoded string) ([]ssh.PublicKey, []byte, error) { + if encoded == "" { + return nil, nil, nil + } + + t, err := token.ParseToken(encoded) + if err != nil { + return nil, nil, fmt.Errorf("parse token: %w", err) + } + + keys, err := decodeAuthorizedKeys(t.AuthorizedKeys) + if err != nil { + return nil, nil, err + } + + hostKey, err := decodeBase64Bytes(t.HostKey, "host key") + if err != nil { + return nil, nil, err + } + + return keys, hostKey, nil +} + +func decodeAuthorizedKeys(encoded string) ([]ssh.PublicKey, error) { + if encoded == "" { + return nil, nil + } + raw, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("decode authorized keys: %w", err) + } + var keys []ssh.PublicKey + for len(raw) > 0 { + key, _, _, rest, err := ssh.ParseAuthorizedKey(raw) if err != nil { - return fmt.Errorf("address %s already in use: %w", cmd.Address, err) + return nil, fmt.Errorf("parse authorized key: %w", err) } + keys = append(keys, key) + raw = rest + } + return keys, nil +} - log.Infof("address %s already in use", cmd.Address) - return nil +func decodeBase64Bytes(encoded, label string) ([]byte, error) { + if encoded == "" { + return nil, nil } + out, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("decode %s: %w", label, err) + } + return out, nil +} - return server.ListenAndServe() +// runActivityHeartbeat periodically updates the activity file's mtime so +// outside watchers can detect liveness. It exits when ctx is canceled. +// Errors transition between "logged" and "silent" — we log only when the +// failure mode changes so a permanently broken file doesn't spam the log. +func runActivityHeartbeat(ctx context.Context, path string) { + if err := ensureActivityFile(path); err != nil { + log.Errorf("activity heartbeat: ensure file: %v", err) + return + } + + t := time.NewTicker(activityHeartbeatInterval) + defer t.Stop() + + var lastErr error + for { + select { + case <-ctx.Done(): + return + case now := <-t.C: + err := os.Chtimes(path, now, now) + if (err == nil) != (lastErr == nil) { + if err != nil { + log.Errorf("activity heartbeat: %v", err) + } else { + log.Infof("activity heartbeat recovered") + } + } + lastErr = err + } + } +} + +func ensureActivityFile(path string) error { + _, err := os.Stat(path) + if err == nil { + return nil + } + if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("stat: %w", err) + } + if err := os.WriteFile( + path, + nil, + activityFileMode, + ); err != nil { // #nosec G306 -- intentionally world-writable; multiple users update activity + return fmt.Errorf("create: %w", err) + } + if err := os.Chmod(path, activityFileMode); err != nil { // #nosec G302 -- ditto + return fmt.Errorf("chmod: %w", err) + } + return nil } diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go new file mode 100644 index 000000000..35d2a56ea --- /dev/null +++ b/cmd/internal/ssh_server_test.go @@ -0,0 +1,140 @@ +package cmdinternal + +import ( + "context" + "encoding/base64" + "encoding/json" + "errors" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/token" +) + +func encodeTestToken(t *testing.T, tok token.Token) string { + t.Helper() + raw, err := json.Marshal(&tok) + if err != nil { + t.Fatalf("marshal token: %v", err) + } + return base64.StdEncoding.EncodeToString(raw) +} + +const testEd25519PubKey = "ssh-ed25519 " + + "AAAAC3NzaC1lZDI1NTE5AAAAIDuxlhheJj+ON3HxiToVhg+Tj1+/cqLgkBQ8KkKr2T87 " + + "test@example" + +func TestParseSSHTokenEmpty(t *testing.T) { + keys, hostKey, err := parseSSHToken("") + if err != nil || keys != nil || hostKey != nil { + t.Fatalf("expected zeroes, got keys=%v hostKey=%v err=%v", keys, hostKey, err) + } +} + +func TestParseSSHTokenInvalid(t *testing.T) { + _, _, err := parseSSHToken("not-a-valid-token-blob") + if err == nil || !strings.Contains(err.Error(), "parse token") { + t.Fatalf("want parse error, got %v", err) + } +} + +func TestParseSSHTokenWithKeys(t *testing.T) { + encoded := encodeTestToken(t, token.Token{ + AuthorizedKeys: base64.StdEncoding.EncodeToString([]byte(testEd25519PubKey + "\n")), + HostKey: base64.StdEncoding.EncodeToString([]byte("hostkeybytes")), + }) + keys, hostKey, err := parseSSHToken(encoded) + if err != nil { + t.Fatalf("parseSSHToken: %v", err) + } + if len(keys) != 1 { + t.Errorf("want 1 key, got %d", len(keys)) + } + if string(hostKey) != "hostkeybytes" { + t.Errorf("hostKey = %q", hostKey) + } +} + +func TestDecodeAuthorizedKeysMultiple(t *testing.T) { + blob := testEd25519PubKey + "\n" + testEd25519PubKey + "\n" + keys, err := decodeAuthorizedKeys(base64.StdEncoding.EncodeToString([]byte(blob))) + if err != nil { + t.Fatalf("decodeAuthorizedKeys: %v", err) + } + if len(keys) != 2 { + t.Errorf("want 2 keys, got %d", len(keys)) + } +} + +func TestDecodeAuthorizedKeysBadBase64(t *testing.T) { + _, err := decodeAuthorizedKeys("!!!not-base64!!!") + if err == nil || !strings.Contains(err.Error(), "decode authorized keys") { + t.Fatalf("want wrapped decode error, got %v", err) + } +} + +func TestDecodeBase64BytesEmpty(t *testing.T) { + out, err := decodeBase64Bytes("", "host key") + if err != nil || out != nil { + t.Fatalf("want (nil,nil), got (%v,%v)", out, err) + } +} + +func TestDecodeBase64BytesError(t *testing.T) { + _, err := decodeBase64Bytes("!!!", "host key") + if err == nil || !strings.Contains(err.Error(), "decode host key") { + t.Fatalf("want wrapped error with label, got %v", err) + } +} + +func TestEnsureActivityFileCreatesAndIsIdempotent(t *testing.T) { + path := filepath.Join(t.TempDir(), "activity") + if err := ensureActivityFile(path); err != nil { + t.Fatalf("first call: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("expected file to exist: %v", err) + } + // idempotent + if err := ensureActivityFile(path); err != nil { + t.Fatalf("second call: %v", err) + } +} + +func TestEnsureActivityFileSurfacesNonExistErrors(t *testing.T) { + // Path through a non-directory is ENOTDIR, which is neither nil nor ErrNotExist — + // confirm we surface it instead of trying to create. + tmp := t.TempDir() + regular := filepath.Join(tmp, "regular") + if err := os.WriteFile(regular, nil, 0o600); err != nil { + t.Fatal(err) + } + bad := filepath.Join(regular, "child") + err := ensureActivityFile(bad) + if err == nil { + t.Fatal("expected error for path under a regular file") + } + if errors.Is(err, fs.ErrNotExist) { + t.Errorf("want non-ErrNotExist error surfaced, got %v", err) + } +} + +func TestRunActivityHeartbeatExitsOnContextCancel(t *testing.T) { + path := filepath.Join(t.TempDir(), "activity") + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + runActivityHeartbeat(ctx, path) + close(done) + }() + cancel() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("heartbeat did not exit within 2s of context cancel") + } +} From 467cfe0e6db252c57cd6e4c0e1b328197ed38e90 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 11:30:06 -0500 Subject: [PATCH 10/11] refactor(ssh-server): graceful shutdown on context cancel + tighter API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add Shutdown(ctx) to the sshserver.Server interface; implement on both the host server and the container server by delegating to the underlying ssh.Server.Shutdown. - Wire into cmd/internal: a goroutine waits on ctx.Done and calls Shutdown with a 5s timeout, so the SSH server actually drains on SIGTERM instead of running until process exit. The shutdown context derives from Background by design — the parent ctx is already canceled by the time we get here. - Wrap the server-closed error: ssh.ErrServerClosed is the expected outcome of a graceful shutdown, not a CLI failure. - Lower SSHServerCmd to unexported sshServerCmd — nothing outside the package needed it. --- cmd/internal/ssh_server.go | 89 +++++++++++++++++++++------------ cmd/internal/ssh_server_test.go | 46 +++++++++++++++++ pkg/ssh/server/ssh.go | 9 ++++ pkg/ssh/server/ssh_container.go | 5 ++ 4 files changed, 118 insertions(+), 31 deletions(-) diff --git a/cmd/internal/ssh_server.go b/cmd/internal/ssh_server.go index c02cf6299..da9a334bd 100644 --- a/cmd/internal/ssh_server.go +++ b/cmd/internal/ssh_server.go @@ -25,54 +25,57 @@ const ( activityFileMode = 0o666 ) -// SSHServerCmd holds the ssh server cmd flags. -type SSHServerCmd struct { +// shutdownTimeout bounds how long we wait for in-flight SSH connections to +// drain on context cancel before falling back to immediate Close(). +const shutdownTimeout = 5 * time.Second + +// sshServerCmd holds the ssh server cmd flags. +type sshServerCmd struct { *flags.GlobalFlags - Token string - Address string - Stdio bool - TrackActivity bool - ReuseSSHAuthSock string - Workdir string + token string + address string + stdio bool + trackActivity bool + reuseSSHAuthSock string + workdir string } // NewSSHServerCmd creates a new ssh command. func NewSSHServerCmd(globalFlags *flags.GlobalFlags) *cobra.Command { - cmd := &SSHServerCmd{GlobalFlags: globalFlags} + cmd := &sshServerCmd{GlobalFlags: globalFlags} sshCmd := &cobra.Command{ Use: "ssh-server", Short: "Starts a new SSH server", Args: cobra.NoArgs, RunE: func(cobraCmd *cobra.Command, _ []string) error { - return cmd.Run(cobraCmd.Context()) + return cmd.run(cobraCmd.Context()) }, } f := sshCmd.Flags() - f.StringVar(&cmd.Address, "address", + f.StringVar(&cmd.address, "address", fmt.Sprintf("0.0.0.0:%d", sshserver.DefaultPort), "Address to listen to") - f.BoolVar(&cmd.Stdio, "stdio", false, + f.BoolVar(&cmd.stdio, "stdio", false, "Listen on stdin/stdout instead of an address") - f.BoolVar(&cmd.TrackActivity, "track-activity", false, + f.BoolVar(&cmd.trackActivity, "track-activity", false, "Touch the activity file every "+activityHeartbeatInterval.String()+" (only with --stdio)") - f.StringVar(&cmd.ReuseSSHAuthSock, "reuse-ssh-auth-sock", "", + f.StringVar(&cmd.reuseSSHAuthSock, "reuse-ssh-auth-sock", "", "If set, reuse a pre-existing SSH_AUTH_SOCK in the workspace under /tmp") _ = f.MarkHidden("reuse-ssh-auth-sock") - f.StringVar(&cmd.Token, "token", "", "Base64 encoded token to use") - f.StringVar(&cmd.Workdir, "workdir", "", + f.StringVar(&cmd.token, "token", "", "Base64 encoded token to use") + f.StringVar(&cmd.workdir, "workdir", "", "Directory where commands will run on the host") return sshCmd } -// Run runs the command logic. -func (cmd *SSHServerCmd) Run(ctx context.Context) error { - if cmd.TrackActivity && !cmd.Stdio { +func (cmd *sshServerCmd) run(ctx context.Context) error { + if cmd.trackActivity && !cmd.stdio { return errors.New("--track-activity requires --stdio") } - keys, hostKey, err := parseSSHToken(cmd.Token) + keys, hostKey, err := parseSSHToken(cmd.token) if err != nil { return err } @@ -85,35 +88,59 @@ func (cmd *SSHServerCmd) Run(ctx context.Context) error { sshserver.SweepStaleAgentSockets() server, err := sshserver.NewServer( - cmd.Address, hostKey, keys, cmd.Workdir, cmd.ReuseSSHAuthSock, + cmd.address, hostKey, keys, cmd.workdir, cmd.reuseSSHAuthSock, ) if err != nil { return fmt.Errorf("create ssh server: %w", err) } - if cmd.Stdio { + if cmd.stdio { return cmd.serveStdio(ctx, server) } - return cmd.serveListener(server) + return cmd.serveListener(ctx, server) } -func (cmd *SSHServerCmd) serveStdio(ctx context.Context, server sshserver.Server) error { - if cmd.TrackActivity { +func (cmd *sshServerCmd) serveStdio(ctx context.Context, server sshserver.Server) error { + if cmd.trackActivity { go runActivityHeartbeat(ctx, config.ContainerActivityFile) } + go shutdownOnCancel(ctx, server) // #nosec G118 -- see shutdownOnCancel. lis := stdio.NewStdioListener(os.Stdin, os.Stdout, true) - return server.Serve(lis) + return ignoreServerClosed(server.Serve(lis)) } -func (cmd *SSHServerCmd) serveListener(server sshserver.Server) error { - available, err := port.IsAvailable(cmd.Address) +func (cmd *sshServerCmd) serveListener(ctx context.Context, server sshserver.Server) error { + available, err := port.IsAvailable(cmd.address) if err != nil { - return fmt.Errorf("check port %s: %w", cmd.Address, err) + return fmt.Errorf("check port %s: %w", cmd.address, err) } if !available { - return fmt.Errorf("address %s already in use", cmd.Address) + return fmt.Errorf("address %s already in use", cmd.address) + } + go shutdownOnCancel(ctx, server) // #nosec G118 -- see shutdownOnCancel. + return ignoreServerClosed(server.ListenAndServe()) +} + +// shutdownOnCancel drains active SSH connections when ctx is canceled, +// bounded by shutdownTimeout. The shutdown context derives from +// context.Background by design: ctx is already canceled at this point, so +// reusing it would make Shutdown's grace period instantly expire. +func shutdownOnCancel(ctx context.Context, server sshserver.Server) { + <-ctx.Done() + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) + defer cancel() + if err := server.Shutdown(shutdownCtx); err != nil { + log.Errorf("ssh server shutdown: %v", err) + } +} + +// ignoreServerClosed turns the expected post-Shutdown error into a clean +// return so cobra doesn't surface "ssh: Server closed" as a failure. +func ignoreServerClosed(err error) error { + if err == nil || errors.Is(err, ssh.ErrServerClosed) { + return nil } - return server.ListenAndServe() + return err } // parseSSHToken decodes the optional base64-encoded token blob. An empty diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go index 35d2a56ea..bdda831af 100644 --- a/cmd/internal/ssh_server_test.go +++ b/cmd/internal/ssh_server_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "io/fs" + "net" "os" "path/filepath" "strings" @@ -13,6 +14,7 @@ import ( "time" "github.com/devsy-org/devsy/pkg/token" + "github.com/devsy-org/ssh" ) func encodeTestToken(t *testing.T, tok token.Token) string { @@ -123,6 +125,50 @@ func TestEnsureActivityFileSurfacesNonExistErrors(t *testing.T) { } } +type fakeServer struct { + shutdownCalls int + shutdownErr error +} + +func (*fakeServer) Serve(net.Listener) error { return nil } +func (*fakeServer) ListenAndServe() error { return nil } +func (f *fakeServer) Shutdown(context.Context) error { + f.shutdownCalls++ + return f.shutdownErr +} + +func TestShutdownOnCancelInvokesServer(t *testing.T) { + f := &fakeServer{} + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + shutdownOnCancel(ctx, f) + close(done) + }() + cancel() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("shutdownOnCancel did not return after ctx cancel") + } + if f.shutdownCalls != 1 { + t.Errorf("Shutdown calls = %d, want 1", f.shutdownCalls) + } +} + +func TestIgnoreServerClosed(t *testing.T) { + if err := ignoreServerClosed(nil); err != nil { + t.Errorf("nil should map to nil, got %v", err) + } + if err := ignoreServerClosed(ssh.ErrServerClosed); err != nil { + t.Errorf("ErrServerClosed should map to nil, got %v", err) + } + other := errors.New("boom") + if err := ignoreServerClosed(other); err != other { + t.Errorf("unrelated error should pass through, got %v", err) + } +} + func TestRunActivityHeartbeatExitsOnContextCancel(t *testing.T) { path := filepath.Join(t.TempDir(), "activity") ctx, cancel := context.WithCancel(context.Background()) diff --git a/pkg/ssh/server/ssh.go b/pkg/ssh/server/ssh.go index 5e61da49b..ab4a5adb3 100644 --- a/pkg/ssh/server/ssh.go +++ b/pkg/ssh/server/ssh.go @@ -1,6 +1,7 @@ package server import ( + "context" "crypto/rand" "crypto/sha256" "encoding/hex" @@ -94,6 +95,10 @@ const ( type Server interface { Serve(listener net.Listener) error ListenAndServe() error + // Shutdown gracefully closes the listener and waits for active + // connections to finish, bounded by ctx. Use Close for an immediate + // termination. + Shutdown(ctx context.Context) error } type server struct { @@ -375,6 +380,10 @@ func (s *server) ListenAndServe() error { return s.sshServer.ListenAndServe() } +func (s *server) Shutdown(ctx context.Context) error { + return s.sshServer.Shutdown(ctx) +} + // connCallback is invoked once per inbound SSH connection. Outside the // explicit reuseSock (openvscode backhaul) mode it stores a lightweight // intent on the ssh.Context and schedules a teardown goroutine. The agent diff --git a/pkg/ssh/server/ssh_container.go b/pkg/ssh/server/ssh_container.go index ffe6350ec..ac5cdaeda 100644 --- a/pkg/ssh/server/ssh_container.go +++ b/pkg/ssh/server/ssh_container.go @@ -1,6 +1,7 @@ package server import ( + "context" "fmt" "net" "os" @@ -73,6 +74,10 @@ func (s *containerServer) Serve(listener net.Listener) error { return s.sshServer.Serve(listener) } +func (s *containerServer) Shutdown(ctx context.Context) error { + return s.sshServer.Shutdown(ctx) +} + func (s *containerServer) ListenAndServe() error { log.Debugf("Start ssh server on %s", s.sshServer.Addr) return s.sshServer.ListenAndServe() From 985e95dd67e7d095192ee9e0409875a3fdf0477e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 20 Jun 2026 11:31:50 -0500 Subject: [PATCH 11/11] style(comments): rephrase 'we' comments as direct statements --- cmd/internal/ssh_server.go | 10 +++++----- cmd/internal/ssh_server_test.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/internal/ssh_server.go b/cmd/internal/ssh_server.go index da9a334bd..4eb3d342a 100644 --- a/cmd/internal/ssh_server.go +++ b/cmd/internal/ssh_server.go @@ -25,8 +25,8 @@ const ( activityFileMode = 0o666 ) -// shutdownTimeout bounds how long we wait for in-flight SSH connections to -// drain on context cancel before falling back to immediate Close(). +// shutdownTimeout bounds the grace period for in-flight SSH connections to +// drain on context cancel before the listener is force-closed. const shutdownTimeout = 5 * time.Second // sshServerCmd holds the ssh server cmd flags. @@ -200,9 +200,9 @@ func decodeBase64Bytes(encoded, label string) ([]byte, error) { } // runActivityHeartbeat periodically updates the activity file's mtime so -// outside watchers can detect liveness. It exits when ctx is canceled. -// Errors transition between "logged" and "silent" — we log only when the -// failure mode changes so a permanently broken file doesn't spam the log. +// outside watchers can detect liveness. Exits when ctx is canceled. Logs +// only on success/failure transitions so a permanently broken file does +// not spam the log. func runActivityHeartbeat(ctx context.Context, path string) { if err := ensureActivityFile(path); err != nil { log.Errorf("activity heartbeat: ensure file: %v", err) diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go index bdda831af..27aaacc36 100644 --- a/cmd/internal/ssh_server_test.go +++ b/cmd/internal/ssh_server_test.go @@ -109,7 +109,7 @@ func TestEnsureActivityFileCreatesAndIsIdempotent(t *testing.T) { func TestEnsureActivityFileSurfacesNonExistErrors(t *testing.T) { // Path through a non-directory is ENOTDIR, which is neither nil nor ErrNotExist — - // confirm we surface it instead of trying to create. + // must surface as-is instead of falling into the create branch. tmp := t.TempDir() regular := filepath.Join(tmp, "regular") if err := os.WriteFile(regular, nil, 0o600); err != nil {