From 8921af7a66e6fc374e14bacf713dcb3f9a4e32c2 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 19:42:07 -0500 Subject: [PATCH 1/3] refactor(client): redesign workspace_client for clarity and lint compliance Reorganize workspace_client.go: exported methods before unexported, extract helpers to reduce cyclomatic/nesting complexity, deduplicate agent-command plumbing, centralize string literals, wrap dropped errors, and use errors.New for static messages. Fixes a latent nil-deref in the forced machine-delete path. Resolves all golangci-lint issues in the file. --- .../clientimplementation/workspace_client.go | 1068 +++++++++-------- 1 file changed, 544 insertions(+), 524 deletions(-) diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index 16525a680..d24c4ce9d 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -28,6 +28,8 @@ import ( "github.com/gofrs/flock" ) +const commandName = "command" + func NewWorkspaceClient( devsyConfig *config.Config, prov *provider.ProviderConfig, @@ -35,9 +37,10 @@ func NewWorkspaceClient( machine *provider.Machine, ) (client.WorkspaceClient, error) { if workspace.Machine.ID != "" && machine == nil { - return nil, fmt.Errorf("workspace machine is not found") - } else if prov.IsMachineProvider() && workspace.Machine.ID == "" { - return nil, fmt.Errorf("workspace machine ID is empty, but machine provider found") + return nil, errors.New("machine not found") + } + if prov.IsMachineProvider() && workspace.Machine.ID == "" { + return nil, errors.New("machine id empty for machine provider") } return &workspaceClient{ @@ -66,6 +69,10 @@ func (s *workspaceClient) Provider() string { return s.config.Name } +func (s *workspaceClient) Context() string { + return s.workspace.Context +} + func (s *workspaceClient) Workspace() string { s.m.Lock() defer s.m.Unlock() @@ -84,25 +91,21 @@ func (s *workspaceClient) AgentLocal() bool { s.m.Lock() defer s.m.Unlock() - return s.agentLocalLocked() + return s.agentLocal() } func (s *workspaceClient) AgentPath() string { s.m.Lock() defer s.m.Unlock() - return options.ResolveAgentConfig(s.devsyConfig, s.config, s.workspace, s.machine).Path + return s.agentConfig().Path } func (s *workspaceClient) AgentURL() string { s.m.Lock() defer s.m.Unlock() - return options.ResolveAgentConfig(s.devsyConfig, s.config, s.workspace, s.machine).DownloadURL -} - -func (s *workspaceClient) Context() string { - return s.workspace.Context + return s.agentConfig().DownloadURL } func (s *workspaceClient) RefreshOptions( @@ -119,43 +122,9 @@ func (s *workspaceClient) RefreshOptions( } if s.isMachineProvider() { - if s.machine == nil { - return nil - } - - machine, err := options.ResolveAndSaveOptionsMachine( - ctx, - s.devsyConfig, - s.config, - s.machine, - userOptions, - ) - if err != nil { - return err - } - - s.machine = machine - return nil + return s.refreshMachineOptions(ctx, userOptions) } - - workspace, err := options.ResolveAndSaveOptionsWorkspace( - ctx, - s.devsyConfig, - s.config, - s.workspace, - userOptions, - ) - if err != nil { - return fmt.Errorf("resolve and save workspace options: %w", err) - } - - if workspace != nil { - s.workspace = workspace - log.Debugf("refreshed workspace options: workspaceId=%s", s.workspace.ID) - } else { - log.Debug("workspace is nil; not updating workspace options") - } - return nil + return s.refreshWorkspaceOptions(ctx, userOptions) } func (s *workspaceClient) AgentInjectGitCredentials(cliOptions provider.CLIOptions) bool { @@ -181,107 +150,21 @@ func (s *workspaceClient) AgentInfo( return s.compressedAgentInfo(cliOptions) } -func (s *workspaceClient) compressedAgentInfo( - cliOptions provider.CLIOptions, -) (string, *provider.AgentWorkspaceInfo, error) { - agentInfo := s.agentInfo(cliOptions) - - // marshal config - out, err := json.Marshal(agentInfo) - if err != nil { - return "", nil, err - } - - compressed, err := compress.Compress(string(out)) - if err != nil { - return "", nil, err - } - - return compressed, agentInfo, nil -} - -func (s *workspaceClient) agentInfo(cliOptions provider.CLIOptions) *provider.AgentWorkspaceInfo { - // try to load last devcontainer.json - var lastDevContainerConfig *config2.DevContainerConfigWithPath - var workspaceOrigin string - if s.workspace != nil { - result, err := provider.LoadWorkspaceResult(s.workspace.Context, s.workspace.ID) - if err != nil { - log.Debugf("error loading workspace result: error=%v", err) - } else if result != nil { - lastDevContainerConfig = result.DevContainerConfigWithPath - } - - workspaceOrigin = s.workspace.Origin - } - - // build struct - agentInfo := &provider.AgentWorkspaceInfo{ - WorkspaceOrigin: workspaceOrigin, - Workspace: s.workspace, - Machine: s.machine, - LastDevContainerConfig: lastDevContainerConfig, - CLIOptions: cliOptions, - Agent: options.ResolveAgentConfig( - s.devsyConfig, - s.config, - s.workspace, - s.machine, - ), - Options: s.devsyConfig.ProviderOptions(s.Provider()), - } - - // if we are running platform mode - if cliOptions.Platform.Enabled { - agentInfo.Agent.InjectGitCredentials = "true" - agentInfo.Agent.InjectDockerCredentials = "true" - } - - // we don't send any provider options if proxy because these could contain - // sensitive information and we don't want to allow privileged containers that - // have access to the host to save these. - if agentInfo.Agent.Driver != provider.CustomDriver && - (cliOptions.Platform.Enabled || cliOptions.DisableDaemon) { - agentInfo.Options = map[string]config.OptionValue{} - agentInfo.Workspace = provider.CloneWorkspace(agentInfo.Workspace) - agentInfo.Workspace.Provider.Options = map[string]config.OptionValue{} - if agentInfo.Machine != nil { - agentInfo.Machine = provider.CloneMachine(agentInfo.Machine) - agentInfo.Machine.Provider.Options = map[string]config.OptionValue{} - } - } - - // Get the timeout from the context options - agentInfo.InjectTimeout = config.ParseTimeOption( - s.devsyConfig, - config.ContextOptionAgentInjectTimeout, - ) - - // Set registry cache from context option - agentInfo.RegistryCache = s.devsyConfig.ContextOption(config.ContextOptionRegistryCache) - - return agentInfo -} - func (s *workspaceClient) Lock(ctx context.Context) error { if err := s.initLock(); err != nil { - return fmt.Errorf("initializing lock: %w", err) + return fmt.Errorf("init lock: %w", err) } - // try to lock workspace log.Debug("acquire workspace lock") - err := tryLock(ctx, s.workspaceLock, "workspace") - if err != nil { - return fmt.Errorf("error locking workspace: %w", err) + if err := tryLock(ctx, s.workspaceLock, "workspace"); err != nil { + return fmt.Errorf("lock workspace: %w", err) } log.Debug("acquired workspace lock") - // try to lock machine if s.machineLock != nil { log.Debug("acquire machine lock") - err := tryLock(ctx, s.machineLock, "machine") - if err != nil { - return fmt.Errorf("error locking machine: %w", err) + if err := tryLock(ctx, s.machineLock, "machine"); err != nil { + return fmt.Errorf("lock machine: %w", err) } log.Debug("acquired machine lock") } @@ -291,19 +174,19 @@ func (s *workspaceClient) Lock(ctx context.Context) error { func (s *workspaceClient) Unlock() { if err := s.initLock(); err != nil { - log.Warnf("initializing lock: %v", err) + log.Warnf("init lock: %v", err) return } if s.machineLock != nil { if err := s.machineLock.Unlock(); err != nil { - log.Warnf("error unlocking machine: %v", err) + log.Warnf("unlock machine: %v", err) } } if s.workspaceLock != nil { if err := s.workspaceLock.Unlock(); err != nil { - log.Warnf("error unlocking workspace: %v", err) + log.Warnf("unlock workspace: %v", err) } } } @@ -312,31 +195,26 @@ func (s *workspaceClient) Create(ctx context.Context) error { s.m.Lock() defer s.m.Unlock() - // provider doesn't support machines if !s.isMachineProvider() { return nil } - - // check machine state if s.machine == nil { - return fmt.Errorf("machine is not defined") + return errors.New("machine not defined") } - // create machine client machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) if err != nil { return err } - // get status - machineStatus, err := machineClient.Status(ctx, client.StatusOptions{}) + status, err := machineClient.Status(ctx, client.StatusOptions{}) if err != nil { return err - } else if machineStatus != client.StatusNotFound { + } + if status != client.StatusNotFound { return nil } - // create the machine return machineClient.Create(ctx) } @@ -344,85 +222,11 @@ func (s *workspaceClient) Delete(ctx context.Context, opt client.DeleteOptions) s.m.Lock() defer s.m.Unlock() - // parse duration - var gracePeriod *time.Duration - if opt.GracePeriod != "" { - duration, err := time.ParseDuration(opt.GracePeriod) - if err == nil { - gracePeriod = &duration - } - } - - // kill the command after the grace period - if gracePeriod != nil { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, *gracePeriod) - defer cancel() - } - - // should just delete container? - if !s.isMachineProvider() || !s.workspace.Machine.AutoDelete { - isRunning, err := s.isMachineRunning(ctx) - if err != nil { - if !opt.Force { - return err - } - } else if isRunning { - writer := log.Writer(log.LevelInfo) - defer func() { _ = writer.Close() }() - - log.Info("deleting workspace container") - compressed, info, err := s.compressedAgentInfo(provider.CLIOptions{}) - if err != nil { - return fmt.Errorf("agent info") - } - command := fmt.Sprintf( - "%q internal agent workspace delete --workspace-info %q", - info.Agent.Path, - compressed, - ) - if opt.RemoveVolumes { - command += " --remove-volumes" - } - err = RunCommandWithBinaries(CommandOptions{ - Ctx: ctx, - Name: "command", - Command: s.config.Exec.Command, - Context: s.workspace.Context, - Workspace: s.workspace, - Machine: s.machine, - Options: s.devsyConfig.ProviderOptions(s.config.Name), - Config: s.config, - ExtraEnv: map[string]string{ - provider.CommandEnv: command, - }, - Stdin: nil, - Stdout: writer, - Stderr: writer, - }) - if err != nil { - if !opt.Force { - return err - } - - if !errors.Is(err, context.DeadlineExceeded) { - log.Errorf("error deleting container: error=%v", err) - } - } - } - } else if s.machine != nil && s.workspace.Machine.ID != "" && len(s.config.Exec.Delete) > 0 { - // delete machine if config was found - machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) - if err != nil { - if !opt.Force { - return err - } - } + ctx, cancel := s.deleteContext(ctx, opt.GracePeriod) + defer cancel() - err = machineClient.Delete(ctx, opt) - if err != nil { - return err - } + if err := s.deleteInstance(ctx, opt); err != nil { + return err } return DeleteWorkspaceFolder(DeleteWorkspaceFolderParams{ @@ -433,196 +237,454 @@ func (s *workspaceClient) Delete(ctx context.Context, opt client.DeleteOptions) }) } -func (s *workspaceClient) isMachineRunning(ctx context.Context) (bool, error) { - if !s.isMachineProvider() { - return true, nil +func (s *workspaceClient) Start(ctx context.Context) error { + s.m.Lock() + defer s.m.Unlock() + + if !s.isMachineProvider() || s.machine == nil { + return nil } - // delete machine if config was found machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) if err != nil { - return false, err + return err } - // retrieve status - status, err := machineClient.Status(ctx, client.StatusOptions{}) + return machineClient.Start(ctx) +} + +func (s *workspaceClient) Stop(ctx context.Context, opt client.StopOptions) error { + s.m.Lock() + defer s.m.Unlock() + + if !s.isMachineProvider() || !s.workspace.Machine.AutoDelete { + return s.stopContainer(ctx) + } + + machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) if err != nil { - return false, fmt.Errorf("retrieve machine status: %w", err) - } else if status == client.StatusRunning { - return true, nil + return err + } + + return machineClient.Stop(ctx, opt) +} + +func (s *workspaceClient) Command(ctx context.Context, opt client.CommandOptions) error { + environ, err := s.buildEnvironment(opt.Command) + if err != nil { + return err + } + + return RunCommand(RunCommandOptions{ + Ctx: ctx, + Command: s.config.Exec.Command, + Environ: environ, + Stdin: opt.Stdin, + Stdout: opt.Stdout, + Stderr: opt.Stderr, + }) +} + +func (s *workspaceClient) Status( + ctx context.Context, + opt client.StatusOptions, +) (client.Status, error) { + s.m.Lock() + defer s.m.Unlock() + + if s.isMachineProvider() && len(s.config.Exec.Status) > 0 { + return s.machineStatus(ctx, opt) + } + + if opt.ContainerStatus { + return s.getContainerStatus(ctx) + } + + return s.workspaceFolderStatus() +} + +func (s *workspaceClient) Describe(ctx context.Context) (string, error) { + s.m.Lock() + defer s.m.Unlock() + + if !s.isMachineProvider() || len(s.config.Exec.Describe) == 0 { + return client.DescriptionNotFound, nil + } + if s.machine == nil { + return client.DescriptionNotFound, nil + } + + machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) + if err != nil { + return client.DescriptionNotFound, err + } + + return machineClient.Describe(ctx) +} + +func (s *workspaceClient) agentConfig() provider.ProviderAgentConfig { + return options.ResolveAgentConfig(s.devsyConfig, s.config, s.workspace, s.machine) +} + +func (s *workspaceClient) agentLocal() bool { + return s.agentConfig().Local == config.BoolTrue +} + +func (s *workspaceClient) isMachineProvider() bool { + return len(s.config.Exec.Create) > 0 +} + +func (s *workspaceClient) refreshMachineOptions( + ctx context.Context, + userOptions map[string]string, +) error { + if s.machine == nil { + return nil + } + + machine, err := options.ResolveAndSaveOptionsMachine( + ctx, + s.devsyConfig, + s.config, + s.machine, + userOptions, + ) + if err != nil { + return err + } + + s.machine = machine + return nil +} + +func (s *workspaceClient) refreshWorkspaceOptions( + ctx context.Context, + userOptions map[string]string, +) error { + workspace, err := options.ResolveAndSaveOptionsWorkspace( + ctx, + s.devsyConfig, + s.config, + s.workspace, + userOptions, + ) + if err != nil { + return fmt.Errorf("resolve and save workspace options: %w", err) + } + + if workspace == nil { + log.Debug("workspace is nil; not updating workspace options") + return nil + } + + s.workspace = workspace + log.Debugf("refreshed workspace options: workspaceId=%s", s.workspace.ID) + return nil +} + +func (s *workspaceClient) compressedAgentInfo( + cliOptions provider.CLIOptions, +) (string, *provider.AgentWorkspaceInfo, error) { + agentInfo := s.agentInfo(cliOptions) + + out, err := json.Marshal(agentInfo) + if err != nil { + return "", nil, err + } + + compressed, err := compress.Compress(string(out)) + if err != nil { + return "", nil, err + } + + return compressed, agentInfo, nil +} + +func (s *workspaceClient) agentInfo(cliOptions provider.CLIOptions) *provider.AgentWorkspaceInfo { + agentInfo := &provider.AgentWorkspaceInfo{ + WorkspaceOrigin: s.workspaceOrigin(), + Workspace: s.workspace, + Machine: s.machine, + LastDevContainerConfig: s.lastDevContainerConfig(), + CLIOptions: cliOptions, + Agent: s.agentConfig(), + Options: s.devsyConfig.ProviderOptions(s.Provider()), + InjectTimeout: config.ParseTimeOption( + s.devsyConfig, + config.ContextOptionAgentInjectTimeout, + ), + RegistryCache: s.devsyConfig.ContextOption(config.ContextOptionRegistryCache), + } + + if cliOptions.Platform.Enabled { + agentInfo.Agent.InjectGitCredentials = config.BoolTrue + agentInfo.Agent.InjectDockerCredentials = config.BoolTrue + } + + if agentInfo.Agent.Driver != provider.CustomDriver && + (cliOptions.Platform.Enabled || cliOptions.DisableDaemon) { + stripProviderOptions(agentInfo) + } + + return agentInfo +} + +func (s *workspaceClient) workspaceOrigin() string { + if s.workspace == nil { + return "" + } + return s.workspace.Origin +} + +func (s *workspaceClient) lastDevContainerConfig() *config2.DevContainerConfigWithPath { + if s.workspace == nil { + return nil + } + + result, err := provider.LoadWorkspaceResult(s.workspace.Context, s.workspace.ID) + if err != nil { + log.Debugf("load workspace result: %v", err) + return nil + } + if result == nil { + return nil + } + + return result.DevContainerConfigWithPath +} + +func (s *workspaceClient) deleteContext( + ctx context.Context, + gracePeriod string, +) (context.Context, context.CancelFunc) { + if gracePeriod == "" { + return context.WithCancel(ctx) + } + + duration, err := time.ParseDuration(gracePeriod) + if err != nil { + return context.WithCancel(ctx) + } + + return context.WithTimeout(ctx, duration) +} + +func (s *workspaceClient) deleteInstance(ctx context.Context, opt client.DeleteOptions) error { + if !s.isMachineProvider() || !s.workspace.Machine.AutoDelete { + return s.deleteContainer(ctx, opt) + } + + if s.machine != nil && s.workspace.Machine.ID != "" && len(s.config.Exec.Delete) > 0 { + return s.deleteMachine(ctx, opt) + } + + return nil +} + +func (s *workspaceClient) deleteContainer(ctx context.Context, opt client.DeleteOptions) error { + isRunning, err := s.isMachineRunning(ctx) + if err != nil { + if opt.Force { + return nil + } + return err + } + if !isRunning { + return nil + } + + writer := log.Writer(log.LevelInfo) + defer func() { _ = writer.Close() }() + + log.Info("deleting workspace container") + command, err := s.agentWorkspaceCommand("delete") + if err != nil { + return err + } + if opt.RemoveVolumes { + command += " --remove-volumes" + } + + return handleForceError(s.runProviderCommand(ctx, command, writer, writer), opt.Force) +} + +func handleForceError(err error, force bool) error { + if err == nil || !force { + return err + } + if !errors.Is(err, context.DeadlineExceeded) { + log.Errorf("delete container: %v", err) + } + return nil +} + +func (s *workspaceClient) deleteMachine(ctx context.Context, opt client.DeleteOptions) error { + machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) + if err != nil { + if !opt.Force { + return err + } + return nil } - return false, nil + return machineClient.Delete(ctx, opt) } -func (s *workspaceClient) Start(ctx context.Context) error { - s.m.Lock() - defer s.m.Unlock() +func (s *workspaceClient) stopContainer(ctx context.Context) error { + writer := log.Writer(log.LevelInfo) + defer func() { _ = writer.Close() }() - if !s.isMachineProvider() || s.machine == nil { - return nil + log.Info("stopping container") + command, err := s.agentWorkspaceCommand("stop") + if err != nil { + return err } - machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) - if err != nil { + if err := s.runProviderCommand(ctx, command, writer, writer); err != nil { return err } + log.Info("stopped container") - return machineClient.Start(ctx) + return nil } -func (s *workspaceClient) Stop(ctx context.Context, opt client.StopOptions) error { - s.m.Lock() - defer s.m.Unlock() - - if !s.isMachineProvider() || !s.workspace.Machine.AutoDelete { - writer := log.Writer(log.LevelInfo) - defer func() { _ = writer.Close() }() - - log.Info("stopping container") - compressed, info, err := s.compressedAgentInfo(provider.CLIOptions{}) - if err != nil { - return fmt.Errorf("agent info") - } - command := fmt.Sprintf( - "%q internal agent workspace stop --workspace-info %q", - info.Agent.Path, - compressed, - ) - err = RunCommandWithBinaries(CommandOptions{ - Ctx: ctx, - Name: "command", - Command: s.config.Exec.Command, - Context: s.workspace.Context, - Workspace: s.workspace, - Machine: s.machine, - Options: s.devsyConfig.ProviderOptions(s.config.Name), - Config: s.config, - ExtraEnv: map[string]string{ - provider.CommandEnv: command, - }, - Stdin: nil, - Stdout: writer, - Stderr: writer, - }) - if err != nil { - return err - } - log.Info("stopped container") - - return nil +func (s *workspaceClient) isMachineRunning(ctx context.Context) (bool, error) { + if !s.isMachineProvider() { + return true, nil } machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) if err != nil { - return err + return false, err } - return machineClient.Stop(ctx, opt) -} - -func (s *workspaceClient) Command( - ctx context.Context, - commandOptions client.CommandOptions, -) (err error) { - environ, err := s.buildEnvironment(commandOptions.Command) + status, err := machineClient.Status(ctx, client.StatusOptions{}) if err != nil { - return err + return false, fmt.Errorf("retrieve machine status: %w", err) } - return RunCommand(RunCommandOptions{ - Ctx: ctx, - Command: s.config.Exec.Command, - Environ: environ, - Stdin: commandOptions.Stdin, - Stdout: commandOptions.Stdout, - Stderr: commandOptions.Stderr, - }) + return status == client.StatusRunning, nil } -func (s *workspaceClient) Status( +func (s *workspaceClient) machineStatus( ctx context.Context, - options client.StatusOptions, + opt client.StatusOptions, ) (client.Status, error) { - s.m.Lock() - defer s.m.Unlock() - - // check if provider has status command - if s.isMachineProvider() && len(s.config.Exec.Status) > 0 { - if s.machine == nil { - return client.StatusNotFound, nil - } - - machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) - if err != nil { - return client.StatusNotFound, err - } - - status, err := machineClient.Status(ctx, options) - if err != nil { - return status, err - } + if s.machine == nil { + return client.StatusNotFound, nil + } - // try to check container status and if that fails check workspace folder - if status == client.StatusRunning && options.ContainerStatus { - return s.getContainerStatus(ctx) - } + machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) + if err != nil { + return client.StatusNotFound, err + } + status, err := machineClient.Status(ctx, opt) + if err != nil { return status, err } - // try to check container status and if that fails check workspace folder - if options.ContainerStatus { + if status == client.StatusRunning && opt.ContainerStatus { return s.getContainerStatus(ctx) } - // logic: - // - if workspace folder exists -> Running - // - if workspace folder doesn't exist -> NotFound + return status, nil +} + +func (s *workspaceClient) workspaceFolderStatus() (client.Status, error) { workspaceFolder, err := provider.GetWorkspaceDir(s.workspace.Context, s.workspace.ID) if err != nil { return "", err } - // does workspace folder exist? - _, err = os.Stat(workspaceFolder) - if err == nil { + if _, err := os.Stat(workspaceFolder); err == nil { return client.StatusRunning, nil } return client.StatusNotFound, nil } -func (s *workspaceClient) Describe(ctx context.Context) (string, error) { - s.m.Lock() - defer s.m.Unlock() +func (s *workspaceClient) getContainerStatus(ctx context.Context) (client.Status, error) { + stdout := &bytes.Buffer{} + buf := &bytes.Buffer{} - // check if provider has 'describe' command - if s.isMachineProvider() && len(s.config.Exec.Describe) > 0 { - if s.machine == nil { - return client.DescriptionNotFound, nil - } + command, err := s.agentWorkspaceCommand("status") + if err != nil { + return "", err + } - machineClient, err := NewMachineClient(s.devsyConfig, s.config, s.machine) - if err != nil { - return client.DescriptionNotFound, err - } + if err := s.runProviderCommand(ctx, command, io.MultiWriter(stdout, buf), buf); err != nil { + return client.StatusNotFound, fmt.Errorf( + "retrieve container status: %s%w", + buf.String(), + err, + ) + } + + parsed, err := client.ParseStatus(stdout.String()) + if err != nil { + return client.StatusNotFound, fmt.Errorf( + "parse container status: %s%w", + buf.String(), + err, + ) + } + + log.Debugf( + "container status: stdout=%s, stderr=%s, parsed=%v", + stdout.String(), + buf.String(), + parsed, + ) + return parsed, nil +} - return machineClient.Describe(ctx) +func (s *workspaceClient) agentWorkspaceCommand(subcommand string) (string, error) { + compressed, info, err := s.compressedAgentInfo(provider.CLIOptions{}) + if err != nil { + return "", fmt.Errorf("agent info: %w", err) } - return client.DescriptionNotFound, nil + return fmt.Sprintf( + "%q internal agent workspace %s --workspace-info %q", + info.Agent.Path, + subcommand, + compressed, + ), nil } -// agentLocalLocked is AgentLocal without re-acquiring s.m; callers must -// already hold the lock. -func (s *workspaceClient) agentLocalLocked() bool { - return options.ResolveAgentConfig( - s.devsyConfig, - s.config, - s.workspace, - s.machine, - ).Local == config.BoolTrue +func (s *workspaceClient) runProviderCommand( + ctx context.Context, + command string, + stdout, stderr io.Writer, +) error { + return RunCommandWithBinaries(CommandOptions{ + Ctx: ctx, + Name: commandName, + Command: s.config.Exec.Command, + Context: s.workspace.Context, + Workspace: s.workspace, + Machine: s.machine, + Options: s.devsyConfig.ProviderOptions(s.config.Name), + Config: s.config, + ExtraEnv: map[string]string{provider.CommandEnv: command}, + Stdout: stdout, + Stderr: stderr, + }) +} + +func (s *workspaceClient) buildEnvironment(command string) ([]string, error) { + s.m.Lock() + defer s.m.Unlock() + + return provider.ToEnvironmentWithBinaries(provider.EnvironmentOptions{ + Context: s.workspace.Context, + Workspace: s.workspace, + Machine: s.machine, + Options: s.devsyConfig.ProviderOptions(s.config.Name), + Config: s.config, + ExtraEnv: map[string]string{provider.CommandEnv: command}, + }) } func (s *workspaceClient) initLock() error { @@ -630,7 +692,6 @@ func (s *workspaceClient) initLock() error { s.m.Lock() defer s.m.Unlock() - // get locks dir workspaceLocksDir, err := provider.GetLocksDir(s.workspace.Context) if err != nil { s.workspaceLockErr = fmt.Errorf("get workspaces dir: %w", err) @@ -641,12 +702,10 @@ func (s *workspaceClient) initLock() error { return } - // create workspace lock s.workspaceLock = flock.New( filepath.Join(workspaceLocksDir, s.workspace.ID+".workspace.lock"), ) - // create machine lock if s.machine != nil { s.machineLock = flock.New( filepath.Join(workspaceLocksDir, s.machine.ID+".machine.lock"), @@ -656,62 +715,15 @@ func (s *workspaceClient) initLock() error { return s.workspaceLockErr } -func (s *workspaceClient) getContainerStatus(ctx context.Context) (client.Status, error) { - stdout := &bytes.Buffer{} - buf := &bytes.Buffer{} - compressed, info, err := s.compressedAgentInfo(provider.CLIOptions{}) - if err != nil { - return "", fmt.Errorf("get agent info") - } - command := fmt.Sprintf( - "%q internal agent workspace status --workspace-info %q", - info.Agent.Path, - compressed, - ) - err = RunCommandWithBinaries(CommandOptions{ - Ctx: ctx, - Name: "command", - Command: s.config.Exec.Command, - Context: s.workspace.Context, - Workspace: s.workspace, - Machine: s.machine, - Options: s.devsyConfig.ProviderOptions(s.config.Name), - Config: s.config, - ExtraEnv: map[string]string{ - provider.CommandEnv: command, - }, - Stdin: nil, - Stdout: io.MultiWriter(stdout, buf), - Stderr: buf, - }) - if err != nil { - return client.StatusNotFound, fmt.Errorf( - "error retrieving container status: %s%w", - buf.String(), - err, - ) - } +func stripProviderOptions(info *provider.AgentWorkspaceInfo) { + info.Options = map[string]config.OptionValue{} + info.Workspace = provider.CloneWorkspace(info.Workspace) + info.Workspace.Provider.Options = map[string]config.OptionValue{} - parsed, err := client.ParseStatus(stdout.String()) - if err != nil { - return client.StatusNotFound, fmt.Errorf( - "error parsing container status: %s%w", - buf.String(), - err, - ) + if info.Machine != nil { + info.Machine = provider.CloneMachine(info.Machine) + info.Machine.Provider.Options = map[string]config.OptionValue{} } - - log.Debugf( - "container status command output: stdout=%s, stderr=%s, parsed=%v", - stdout.String(), - buf.String(), - parsed, - ) - return parsed, nil -} - -func (s *workspaceClient) isMachineProvider() bool { - return len(s.config.Exec.Create) > 0 } type CommandOptions struct { @@ -729,22 +741,6 @@ type CommandOptions struct { Stderr io.Writer } -func (s *workspaceClient) buildEnvironment(command string) ([]string, error) { - s.m.Lock() - defer s.m.Unlock() - - return provider.ToEnvironmentWithBinaries(provider.EnvironmentOptions{ - Context: s.workspace.Context, - Workspace: s.workspace, - Machine: s.machine, - Options: s.devsyConfig.ProviderOptions(s.config.Name), - Config: s.config, - ExtraEnv: map[string]string{ - provider.CommandEnv: command, - }, - }) -} - func RunCommandWithBinaries(opts CommandOptions) error { environ, err := provider.ToEnvironmentWithBinaries(provider.EnvironmentOptions{ Context: opts.Context, @@ -786,7 +782,6 @@ func RunCommand(opts RunCommandOptions) error { opts.Environ = append(opts.Environ, config.EnvDebug+"="+config.BoolTrue) } - // use shell if command length is equal 1 if len(opts.Command) == 1 { return shell.RunEmulatedShell( opts.Ctx, @@ -798,18 +793,12 @@ func RunCommand(opts RunCommandOptions) error { ) } - // run command - cmd := exec.CommandContext(opts.Ctx, opts.Command[0], opts.Command[1:]...) + cmd := exec.CommandContext(opts.Ctx, opts.Command[0], opts.Command[1:]...) // #nosec G204 cmd.Stdin = opts.Stdin cmd.Stdout = opts.Stdout cmd.Stderr = opts.Stderr cmd.Env = opts.Environ - err := cmd.Run() - if err != nil { - return err - } - - return nil + return cmd.Run() } func DeleteMachineFolder(context, machineID string) error { @@ -818,13 +807,7 @@ func DeleteMachineFolder(context, machineID string) error { return err } - // remove machine folder - err = os.RemoveAll(machineDir) - if err != nil && !os.IsNotExist(err) { - return err - } - - return nil + return removeAll(machineDir) } type DeleteWorkspaceFolderParams struct { @@ -835,46 +818,64 @@ type DeleteWorkspaceFolderParams struct { } func DeleteWorkspaceFolder(params DeleteWorkspaceFolderParams) error { - path, err := ssh.ResolveSSHConfigPath(params.SSHConfigPath) + if err := removeSSHConfig(params); err != nil { + return err + } + + if err := removeWorkspaceDir(params.Context, params.WorkspaceID); err != nil { + return err + } + + return removeWorkspaceContent(params.Context, params.WorkspaceID) +} + +func removeSSHConfig(params DeleteWorkspaceFolderParams) error { + sshConfigPath, err := ssh.ResolveSSHConfigPath(params.SSHConfigPath) if err != nil { return err } - sshConfigPath := path sshConfigIncludePath := params.SSHConfigIncludePath if sshConfigIncludePath != "" { - includePath, err := ssh.ResolveSSHConfigPath(sshConfigIncludePath) + sshConfigIncludePath, err = ssh.ResolveSSHConfigPath(sshConfigIncludePath) if err != nil { return err } - sshConfigIncludePath = includePath } - err = ssh.RemoveFromConfig(params.WorkspaceID, sshConfigPath, sshConfigIncludePath) - if err != nil { - log.Errorf("Remove workspace %q from ssh config: %v", params.WorkspaceID, err) + if err := ssh.RemoveFromConfig( + params.WorkspaceID, + sshConfigPath, + sshConfigIncludePath, + ); err != nil { + log.Errorf("remove workspace %q from ssh config: %v", params.WorkspaceID, err) } - workspaceFolder, err := provider.GetWorkspaceDir(params.Context, params.WorkspaceID) + return nil +} + +func removeWorkspaceDir(context, workspaceID string) error { + workspaceFolder, err := provider.GetWorkspaceDir(context, workspaceID) if err != nil { return err } - // remove workspace folder - err = os.RemoveAll(workspaceFolder) - if err != nil && !os.IsNotExist(err) { - return err - } + return removeAll(workspaceFolder) +} - // remove the content folder leaf (outside WorkspaceDir); leave its parent - // "contents" dir in place to keep its host inode stable across recreate. - contentFolder, err := provider.GetWorkspaceContentDir(params.Context, params.WorkspaceID) - if err == nil { - if err := os.RemoveAll(contentFolder); err != nil && !os.IsNotExist(err) { - return err - } +func removeWorkspaceContent(context, workspaceID string) error { + contentFolder, err := provider.GetWorkspaceContentDir(context, workspaceID) + if err != nil { + return nil } + return removeAll(contentFolder) +} + +func removeAll(path string) error { + if err := os.RemoveAll(path); err != nil && !os.IsNotExist(err) { + return err + } return nil } @@ -884,44 +885,49 @@ const ( ) // StartWait waits for the workspace to be ready, optionally creating/starting it. -func StartWait( - ctx context.Context, - workspaceClient client.WorkspaceClient, - create bool, -) error { +func StartWait(ctx context.Context, workspaceClient client.WorkspaceClient, create bool) error { startWaiting := time.Now() for { - instanceStatus, err := workspaceClient.Status(ctx, client.StatusOptions{}) + done, err := startWaitStep(ctx, workspaceClient, create, &startWaiting) if err != nil { return err } - - switch instanceStatus { - case client.StatusBusy: - if handleBusyStatus(&startWaiting) { - time.Sleep(pollInterval) - continue - } - case client.StatusStopped: - if err := handleStoppedStatus(ctx, workspaceClient, create); err != nil { - return err - } - case client.StatusNotFound: - if err := handleNotFoundStatus(ctx, workspaceClient, create); err != nil { - return err - } - default: + if done { return nil } } } -func handleBusyStatus(startWaiting *time.Time) bool { +func startWaitStep( + ctx context.Context, + workspaceClient client.WorkspaceClient, + create bool, + startWaiting *time.Time, +) (bool, error) { + status, err := workspaceClient.Status(ctx, client.StatusOptions{}) + if err != nil { + return false, err + } + + switch status { + case client.StatusBusy: + logBusy(startWaiting) + time.Sleep(pollInterval) + return false, nil + case client.StatusStopped: + return false, handleStoppedStatus(ctx, workspaceClient, create) + case client.StatusNotFound: + return false, handleNotFoundStatus(ctx, workspaceClient, create) + default: + return true, nil + } +} + +func logBusy(startWaiting *time.Time) { if time.Since(*startWaiting) > logThreshold { - log.Info("workspace is busy, waiting for workspace to become ready") + log.Info("workspace is busy, waiting for it to become ready") *startWaiting = time.Now() } - return true } func handleStoppedStatus( @@ -929,14 +935,13 @@ func handleStoppedStatus( workspaceClient client.WorkspaceClient, create bool, ) error { - if create { - err := workspaceClient.Start(ctx) - if err != nil { - return fmt.Errorf("start workspace: %w", err) - } - return nil + if !create { + return errors.New("workspace is stopped") } - return fmt.Errorf("workspace is stopped") + if err := workspaceClient.Start(ctx); err != nil { + return fmt.Errorf("start workspace: %w", err) + } + return nil } func handleNotFoundStatus( @@ -944,14 +949,10 @@ func handleNotFoundStatus( workspaceClient client.WorkspaceClient, create bool, ) error { - if create { - err := workspaceClient.Create(ctx) - if err != nil { - return err - } - return nil + if !create { + return errors.New("workspace not found") } - return fmt.Errorf("workspace not found") + return workspaceClient.Create(ctx) } // BuildAgentClientOptions contains parameters for BuildAgentClient. @@ -969,29 +970,27 @@ func BuildAgentClient(ctx context.Context, opts BuildAgentClientOptions) (*confi return nil, err } - command := buildAgentCommand(opts.WorkspaceClient, opts.AgentCommand, workspaceInfo) - stdoutReader, stdoutWriter, stdinReader, stdinWriter, err := createPipes() + pipes, err := newAgentPipes() if err != nil { return nil, err } - defer func() { _ = stdoutWriter.Close() }() - defer func() { _ = stdoutReader.Close() }() - defer func() { _ = stdinReader.Close() }() - defer func() { _ = stdinWriter.Close() }() + defer pipes.close() cancelCtx, cancel := context.WithCancel(ctx) defer cancel() + command := buildAgentCommand(opts.WorkspaceClient, opts.AgentCommand, workspaceInfo) errChan := runAgentInjection(agentInjectionOptions{ ctx: cancelCtx, workspaceClient: opts.WorkspaceClient, command: command, - stdin: stdinReader, - stdout: stdoutWriter, + stdin: pipes.stdinReader, + stdout: pipes.stdoutWriter, timeout: wInfo.InjectTimeout, cancel: cancel, }) - result, err := runTunnelServer(cancelCtx, opts, stdoutReader, stdinWriter) + + result, err := runTunnelServer(cancelCtx, opts, pipes.stdoutReader, pipes.stdinWriter) if err != nil { return nil, err } @@ -1015,18 +1014,39 @@ func buildAgentCommand( return command } -func createPipes() (stdoutReader, stdoutWriter, stdinReader, stdinWriter *os.File, err error) { - stdoutReader, stdoutWriter, err = os.Pipe() +type agentPipes struct { + stdoutReader *os.File + stdoutWriter *os.File + stdinReader *os.File + stdinWriter *os.File +} + +func newAgentPipes() (*agentPipes, error) { + stdoutReader, stdoutWriter, err := os.Pipe() if err != nil { - return nil, nil, nil, nil, err + return nil, err } - stdinReader, stdinWriter, err = os.Pipe() + + stdinReader, stdinWriter, err := os.Pipe() if err != nil { - func() { _ = stdoutReader.Close() }() - func() { _ = stdoutWriter.Close() }() - return nil, nil, nil, nil, err + _ = stdoutReader.Close() + _ = stdoutWriter.Close() + return nil, err } - return stdoutReader, stdoutWriter, stdinReader, stdinWriter, nil + + return &agentPipes{ + stdoutReader: stdoutReader, + stdoutWriter: stdoutWriter, + stdinReader: stdinReader, + stdinWriter: stdinWriter, + }, nil +} + +func (p *agentPipes) close() { + _ = p.stdoutWriter.Close() + _ = p.stdoutReader.Close() + _ = p.stdinReader.Close() + _ = p.stdinWriter.Close() } type agentInjectionOptions struct { From 75e6881f7393ff616c1671b129891a39b7f99050 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 19:59:32 -0500 Subject: [PATCH 2/3] refactor(client): drop dead CommandOptions.Name, fix error format, add tests Remove the unused CommandOptions.Name field and the execParams.name plumbing that only fed it, along with all operation-label setters across cmd/ and pkg/workspace. Add a separator to the container-status wrapped-error format (%s%w -> %s: %w). Add unit tests for the redesigned helpers (handleForceError, deleteContext, removeAll, RunCommand, logBusy, and the StartWait status handlers). --- cmd/pro/cluster/list.go | 1 - cmd/pro/health.go | 1 - cmd/pro/project/list.go | 1 - cmd/pro/self.go | 1 - cmd/pro/template/list.go | 1 - cmd/pro/version.go | 1 - cmd/pro/workspace/create.go | 1 - cmd/pro/workspace/list.go | 1 - cmd/pro/workspace/update.go | 1 - cmd/pro/workspace/watch.go | 1 - cmd/provider/configure_shared.go | 1 - .../clientimplementation/machine_client.go | 1 - .../clientimplementation/proxy_client.go | 9 - .../clientimplementation/workspace_client.go | 8 +- .../workspace_client_test.go | 199 ++++++++++++++++++ pkg/workspace/list.go | 1 - 16 files changed, 201 insertions(+), 28 deletions(-) create mode 100644 pkg/client/clientimplementation/workspace_client_test.go diff --git a/cmd/pro/cluster/list.go b/cmd/pro/cluster/list.go index c6561e74a..80ed5ca49 100644 --- a/cmd/pro/cluster/list.go +++ b/cmd/pro/cluster/list.go @@ -72,7 +72,6 @@ func (cmd *ListClustersCmd) Run( var buf bytes.Buffer err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "listClusters", Command: provider.Exec.Proxy.List.Clusters, Context: devsyConfig.DefaultContext, Options: opts, diff --git a/cmd/pro/health.go b/cmd/pro/health.go index 7632596c9..32d044a14 100644 --- a/cmd/pro/health.go +++ b/cmd/pro/health.go @@ -77,7 +77,6 @@ func (cmd *HealthCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "health", Command: provider.Exec.Proxy.Health, Context: devsyConfig.DefaultContext, Options: devsyConfig.ProviderOptions(provider.Name), diff --git a/cmd/pro/project/list.go b/cmd/pro/project/list.go index 4bb79642a..2d6f6bd1e 100644 --- a/cmd/pro/project/list.go +++ b/cmd/pro/project/list.go @@ -65,7 +65,6 @@ func (cmd *ListProjectsCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "listProjects", Command: provider.Exec.Proxy.List.Projects, Context: devsyConfig.DefaultContext, Options: devsyConfig.ProviderOptions(provider.Name), diff --git a/cmd/pro/self.go b/cmd/pro/self.go index 1dc7e098d..ae85b28bc 100644 --- a/cmd/pro/self.go +++ b/cmd/pro/self.go @@ -62,7 +62,6 @@ func (cmd *SelfCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "getSelf", Command: provider.Exec.Proxy.Get.Self, Context: devsyConfig.DefaultContext, Options: devsyConfig.ProviderOptions(provider.Name), diff --git a/cmd/pro/template/list.go b/cmd/pro/template/list.go index 252b24efc..1783e8ee8 100644 --- a/cmd/pro/template/list.go +++ b/cmd/pro/template/list.go @@ -72,7 +72,6 @@ func (cmd *ListTemplatesCmd) Run( var buf bytes.Buffer err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "listTemplates", Command: provider.Exec.Proxy.List.Templates, Context: devsyConfig.DefaultContext, Options: opts, diff --git a/cmd/pro/version.go b/cmd/pro/version.go index 04adcd1e2..9a94b6837 100644 --- a/cmd/pro/version.go +++ b/cmd/pro/version.go @@ -66,7 +66,6 @@ func (cmd *VersionCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "getVersion", Command: providerConfig.Exec.Proxy.Get.Version, Context: devsyConfig.DefaultContext, Options: opts, diff --git a/cmd/pro/workspace/create.go b/cmd/pro/workspace/create.go index 570b5179b..c224d5e26 100644 --- a/cmd/pro/workspace/create.go +++ b/cmd/pro/workspace/create.go @@ -75,7 +75,6 @@ func (cmd *CreateWorkspaceCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "createWorkspace", Command: provider.Exec.Proxy.Create.Workspace, Context: devsyConfig.DefaultContext, Options: opts, diff --git a/cmd/pro/workspace/list.go b/cmd/pro/workspace/list.go index 02639275b..2d1199dac 100644 --- a/cmd/pro/workspace/list.go +++ b/cmd/pro/workspace/list.go @@ -66,7 +66,6 @@ func (cmd *ListWorkspacesCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "listWorkspaces", Command: provider.Exec.Proxy.List.Workspaces, Context: devsyConfig.DefaultContext, Options: devsyConfig.ProviderOptions(provider.Name), diff --git a/cmd/pro/workspace/update.go b/cmd/pro/workspace/update.go index 75e4017ae..a3d11640d 100644 --- a/cmd/pro/workspace/update.go +++ b/cmd/pro/workspace/update.go @@ -75,7 +75,6 @@ func (cmd *UpdateWorkspaceCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "updateWorkspace", Command: provider.Exec.Proxy.Update.Workspace, Context: devsyConfig.DefaultContext, Options: opts, diff --git a/cmd/pro/workspace/watch.go b/cmd/pro/workspace/watch.go index ac6c5219d..ed3d92537 100644 --- a/cmd/pro/workspace/watch.go +++ b/cmd/pro/workspace/watch.go @@ -94,7 +94,6 @@ func (cmd *WatchWorkspacesCmd) Run( err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: cancelCtx, - Name: "watchWorkspaces", Command: providerConfig.Exec.Proxy.Watch.Workspaces, Context: devsyConfig.DefaultContext, Options: opts, diff --git a/cmd/provider/configure_shared.go b/cmd/provider/configure_shared.go index e1fccc497..14539f7c1 100644 --- a/cmd/provider/configure_shared.go +++ b/cmd/provider/configure_shared.go @@ -166,7 +166,6 @@ func initProvider( ) error { err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "init", Command: provider.Exec.Init, Context: devsyConfig.DefaultContext, Options: devsyConfig.ProviderOptions(provider.Name), diff --git a/pkg/client/clientimplementation/machine_client.go b/pkg/client/clientimplementation/machine_client.go index e7caaa83b..f678b7392 100644 --- a/pkg/client/clientimplementation/machine_client.go +++ b/pkg/client/clientimplementation/machine_client.go @@ -78,7 +78,6 @@ func (e *machineExecutor) execute(ctx context.Context, cfg execConfig) error { err := RunCommandWithBinaries(CommandOptions{ Ctx: ctx, - Name: cfg.name, Command: cfg.command, Context: e.client.machine.Context, Machine: e.client.machine, diff --git a/pkg/client/clientimplementation/proxy_client.go b/pkg/client/clientimplementation/proxy_client.go index 93eed2e70..98871f9f4 100644 --- a/pkg/client/clientimplementation/proxy_client.go +++ b/pkg/client/clientimplementation/proxy_client.go @@ -64,7 +64,6 @@ type proxyExecutor struct { // execParams defines parameters for proxy command execution. type execParams struct { - name string command types.StrArray extraEnv map[string]string stdin io.Reader @@ -76,7 +75,6 @@ type execParams struct { func (e *proxyExecutor) execute(ctx context.Context, params execParams) error { return RunCommandWithBinaries(CommandOptions{ Ctx: ctx, - Name: params.name, Command: params.command, Context: e.client.workspace.Context, Workspace: e.client.workspace, @@ -249,7 +247,6 @@ func (s *proxyClient) Create( stderr io.Writer, ) error { err := s.executor.execute(ctx, execParams{ - name: "createWorkspace", command: s.config.Exec.Proxy.Create.Workspace, stdin: stdin, stdout: stdout, @@ -263,7 +260,6 @@ func (s *proxyClient) Create( func (s *proxyClient) Ssh(ctx context.Context, opt client.SshOptions) error { return s.executor.executeWithJSONLog(ctx, execParams{ - name: "ssh", command: s.config.Exec.Proxy.Ssh, extraEnv: EncodeOptions(opt, config.EnvFlagsSSH), stdin: opt.Stdin, @@ -276,7 +272,6 @@ func (s *proxyClient) Stop(ctx context.Context, opt client.StopOptions) error { defer s.m.Unlock() return s.executor.executeWithJSONLog(ctx, execParams{ - name: "stop", command: s.config.Exec.Proxy.Stop, stdout: io.Discard, }) @@ -294,7 +289,6 @@ func (s *proxyClient) Up(ctx context.Context, opt client.UpOptions) error { } return s.executor.executeWithJSONLog(ctx, execParams{ - name: "up", command: s.config.Exec.Proxy.Up, extraEnv: opts, stdin: opt.Stdin, @@ -350,7 +344,6 @@ func (s *proxyClient) Delete(ctx context.Context, opt client.DeleteOptions) erro } err := s.executor.executeWithJSONLog(ctx, execParams{ - name: "delete", command: s.config.Exec.Proxy.Delete, extraEnv: EncodeOptions(opt, config.EnvFlagsDelete), stdout: io.Discard, @@ -381,7 +374,6 @@ func (s *proxyClient) Status( buf := &bytes.Buffer{} err := RunCommandWithBinaries(CommandOptions{ Ctx: ctx, - Name: "status", Command: s.config.Exec.Proxy.Status, Context: s.workspace.Context, Workspace: s.workspace, @@ -422,7 +414,6 @@ func (s *proxyClient) updateInstance(ctx context.Context) error { } return s.executor.execute(ctx, execParams{ - name: "updateWorkspace", command: s.config.Exec.Proxy.Update.Workspace, stdin: os.Stdin, stdout: os.Stdout, diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index d24c4ce9d..4d8626de9 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -28,8 +28,6 @@ import ( "github.com/gofrs/flock" ) -const commandName = "command" - func NewWorkspaceClient( devsyConfig *config.Config, prov *provider.ProviderConfig, @@ -615,7 +613,7 @@ func (s *workspaceClient) getContainerStatus(ctx context.Context) (client.Status if err := s.runProviderCommand(ctx, command, io.MultiWriter(stdout, buf), buf); err != nil { return client.StatusNotFound, fmt.Errorf( - "retrieve container status: %s%w", + "retrieve container status: %s: %w", buf.String(), err, ) @@ -624,7 +622,7 @@ func (s *workspaceClient) getContainerStatus(ctx context.Context) (client.Status parsed, err := client.ParseStatus(stdout.String()) if err != nil { return client.StatusNotFound, fmt.Errorf( - "parse container status: %s%w", + "parse container status: %s: %w", buf.String(), err, ) @@ -660,7 +658,6 @@ func (s *workspaceClient) runProviderCommand( ) error { return RunCommandWithBinaries(CommandOptions{ Ctx: ctx, - Name: commandName, Command: s.config.Exec.Command, Context: s.workspace.Context, Workspace: s.workspace, @@ -728,7 +725,6 @@ func stripProviderOptions(info *provider.AgentWorkspaceInfo) { type CommandOptions struct { Ctx context.Context - Name string Command types.StrArray Context string Workspace *provider.Workspace diff --git a/pkg/client/clientimplementation/workspace_client_test.go b/pkg/client/clientimplementation/workspace_client_test.go new file mode 100644 index 000000000..903672658 --- /dev/null +++ b/pkg/client/clientimplementation/workspace_client_test.go @@ -0,0 +1,199 @@ +package clientimplementation + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/client" + "github.com/devsy-org/devsy/pkg/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHandleForceError(t *testing.T) { + sentinel := errors.New("boom") + + tests := []struct { + name string + err error + force bool + wantErr error + }{ + {name: "no error", err: nil, force: false, wantErr: nil}, + {name: "no error forced", err: nil, force: true, wantErr: nil}, + {name: "error not forced", err: sentinel, force: false, wantErr: sentinel}, + {name: "error forced", err: sentinel, force: true, wantErr: nil}, + { + name: "deadline forced", + err: context.DeadlineExceeded, + force: true, + wantErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantErr, handleForceError(tt.err, tt.force)) + }) + } +} + +func TestDeleteContext(t *testing.T) { + s := &workspaceClient{} + + t.Run("empty grace period has no deadline", func(t *testing.T) { + ctx, cancel := s.deleteContext(context.Background(), "") + defer cancel() + _, ok := ctx.Deadline() + assert.False(t, ok) + }) + + t.Run("invalid grace period has no deadline", func(t *testing.T) { + ctx, cancel := s.deleteContext(context.Background(), "not-a-duration") + defer cancel() + _, ok := ctx.Deadline() + assert.False(t, ok) + }) + + t.Run("valid grace period sets deadline", func(t *testing.T) { + ctx, cancel := s.deleteContext(context.Background(), "50ms") + defer cancel() + deadline, ok := ctx.Deadline() + require.True(t, ok) + assert.WithinDuration(t, time.Now().Add(50*time.Millisecond), deadline, time.Second) + }) +} + +func TestRemoveAll(t *testing.T) { + t.Run("missing path returns nil", func(t *testing.T) { + assert.NoError(t, removeAll(filepath.Join(t.TempDir(), "does-not-exist"))) + }) + + t.Run("existing tree is removed", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "tree") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "nested"), 0o750)) + + require.NoError(t, removeAll(dir)) + _, err := os.Stat(dir) + assert.True(t, os.IsNotExist(err)) + }) +} + +func TestRunCommand(t *testing.T) { + t.Run("empty command is a no-op", func(t *testing.T) { + assert.NoError(t, RunCommand(RunCommandOptions{Ctx: context.Background()})) + }) + + t.Run("multi-arg command runs via exec", func(t *testing.T) { + stdout := &bytes.Buffer{} + err := RunCommand(RunCommandOptions{ + Ctx: context.Background(), + Command: types.StrArray{"echo", "hello"}, + Stdout: stdout, + }) + require.NoError(t, err) + assert.Equal(t, "hello", strings.TrimSpace(stdout.String())) + }) + + t.Run("single command runs via emulated shell", func(t *testing.T) { + stdout := &bytes.Buffer{} + err := RunCommand(RunCommandOptions{ + Ctx: context.Background(), + Command: types.StrArray{"echo emulated"}, + Stdout: stdout, + }) + require.NoError(t, err) + assert.Equal(t, "emulated", strings.TrimSpace(stdout.String())) + }) +} + +func TestLogBusy(t *testing.T) { + t.Run("within threshold keeps timestamp", func(t *testing.T) { + start := time.Now() + logBusy(&start) + assert.WithinDuration(t, time.Now(), start, time.Second) + }) + + t.Run("past threshold resets timestamp", func(t *testing.T) { + start := time.Now().Add(-2 * logThreshold) + logBusy(&start) + assert.WithinDuration(t, time.Now(), start, time.Second) + }) +} + +func TestStartWaitStatusHandlers(t *testing.T) { + ctx := context.Background() + + t.Run("stopped without create errors", func(t *testing.T) { + err := handleStoppedStatus(ctx, &fakeWorkspaceClient{}, false) + assert.EqualError(t, err, "workspace is stopped") + }) + + t.Run("stopped with create starts workspace", func(t *testing.T) { + fake := &fakeWorkspaceClient{} + require.NoError(t, handleStoppedStatus(ctx, fake, true)) + assert.True(t, fake.startCalled) + }) + + t.Run("stopped start failure is wrapped", func(t *testing.T) { + fake := &fakeWorkspaceClient{startErr: errors.New("nope")} + err := handleStoppedStatus(ctx, fake, true) + assert.ErrorContains(t, err, "start workspace") + }) + + t.Run("not found without create errors", func(t *testing.T) { + err := handleNotFoundStatus(ctx, &fakeWorkspaceClient{}, false) + assert.EqualError(t, err, "workspace not found") + }) + + t.Run("not found with create creates workspace", func(t *testing.T) { + fake := &fakeWorkspaceClient{} + require.NoError(t, handleNotFoundStatus(ctx, fake, true)) + assert.True(t, fake.createCalled) + }) +} + +func TestStartWaitReturnsWhenRunning(t *testing.T) { + fake := &fakeWorkspaceClient{status: client.StatusRunning} + require.NoError(t, StartWait(context.Background(), fake, false)) +} + +func TestStartWaitPropagatesStatusError(t *testing.T) { + fake := &fakeWorkspaceClient{statusErr: errors.New("status failed")} + assert.ErrorContains(t, StartWait(context.Background(), fake, false), "status failed") +} + +type fakeWorkspaceClient struct { + client.WorkspaceClient + + status client.Status + statusErr error + + startCalled bool + startErr error + createCalled bool + createErr error +} + +func (f *fakeWorkspaceClient) Status( + context.Context, + client.StatusOptions, +) (client.Status, error) { + return f.status, f.statusErr +} + +func (f *fakeWorkspaceClient) Start(context.Context) error { + f.startCalled = true + return f.startErr +} + +func (f *fakeWorkspaceClient) Create(context.Context) error { + f.createCalled = true + return f.createErr +} diff --git a/pkg/workspace/list.go b/pkg/workspace/list.go index a0cde653e..82a8bc7b2 100644 --- a/pkg/workspace/list.go +++ b/pkg/workspace/list.go @@ -367,7 +367,6 @@ func listInstancesProxyProvider( if err := clientimplementation.RunCommandWithBinaries(clientimplementation.CommandOptions{ Ctx: ctx, - Name: "listWorkspaces", Command: providerConfig.Exec.Proxy.List.Workspaces, Context: devsyConfig.DefaultContext, Options: opts, From b853ee8747908ae220d54a807f27f120559fa441 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 20:02:36 -0500 Subject: [PATCH 3/3] style: drop comment --- pkg/client/clientimplementation/proxy_client.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/client/clientimplementation/proxy_client.go b/pkg/client/clientimplementation/proxy_client.go index 98871f9f4..49c316730 100644 --- a/pkg/client/clientimplementation/proxy_client.go +++ b/pkg/client/clientimplementation/proxy_client.go @@ -62,7 +62,6 @@ type proxyExecutor struct { client *proxyClient } -// execParams defines parameters for proxy command execution. type execParams struct { command types.StrArray extraEnv map[string]string