Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 88 additions & 37 deletions pkg/cmd/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,61 +176,110 @@ func parseCommand(command string) (string, error) {
const pollTimeout = 10 * time.Minute

func runExecCommand(t *terminal.Terminal, sstore ExecStore, workspaceNameOrID string, host bool, command string) error {
s := t.NewSpinner()
workspace, err := util.GetUserWorkspaceByNameOrIDErr(sstore, workspaceNameOrID)
if err != nil {
return breverrors.WrapAndTrace(err)
// Determine SSH alias: use the workspace name directly (with -host suffix if needed)
sshName := workspaceNameOrID
if host {
sshName = workspaceNameOrID + "-host"
}

// Fire SSH immediately with a short timeout — skip all status checks for speed.
// SSH multiplexing (ControlMaster) in the config means subsequent
// calls reuse an existing connection and are near-instant.
// Use a 5-second connect timeout so we fail fast if the instance is down.
err := runSSHWithTimeout(sshName, command, 5)
if err == nil {
// Success — fire analytics in background and return
go trackExecAnalytics(sstore, workspaceNameOrID)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Goroutine leak / silent failure in trackExecAnalytics

go trackExecAnalytics(sstore, workspaceNameOrID)

trackExecAnalytics makes API calls (GetUserWorkspaceByNameOrIDErr, GetCurrentUser, TrackEvent). If these block or panic, the goroutine will leak or crash silently. The
function also silently swallows all errors.

Suggestion: Consider adding a timeout context or recover() in the goroutine. The silent error swallowing is acceptable for analytics, but a panic would take down the
process.

return nil
}

if workspace.Status == "STOPPED" { // we start the env for the user
err = util.StartWorkspaceIfStopped(t, s, sstore, workspaceNameOrID, workspace, pollTimeout)
// SSH failed — now check what's going on with the instance
fmt.Fprintf(os.Stderr, "Connection failed, checking instance status...\n")

workspace, lookupErr := util.GetUserWorkspaceByNameOrIDErr(sstore, workspaceNameOrID)
if lookupErr != nil {
return breverrors.WrapAndTrace(fmt.Errorf(
"ssh connection failed and could not look up instance %q: %w\nPlease check your instances with: brev ls",
workspaceNameOrID, err))
}

if workspace.Status == "STOPPED" {
s := t.NewSpinner()
startErr := util.StartWorkspaceIfStopped(t, s, sstore, workspaceNameOrID, workspace, pollTimeout)
if startErr != nil {
return breverrors.WrapAndTrace(startErr)
}
err = util.PollUntil(s, workspace.ID, "RUNNING", sstore, " waiting for instance to be ready...", pollTimeout)
if err != nil {
return breverrors.WrapAndTrace(err)
}
// Refresh SSH config so the host entry is up to date
refreshRes := refresh.RunRefreshAsync(sstore)
if err = refreshRes.Await(); err != nil {
return breverrors.WrapAndTrace(err)
}

localIdentifier := workspace.GetLocalIdentifier()
if host {
localIdentifier = workspace.GetHostIdentifier()
}
sshName = string(localIdentifier)

err = util.WaitForSSHToBeAvailable(sshName, s)
if err != nil {
return breverrors.WrapAndTrace(err)
}
_ = writeconnectionevent.WriteWCEOnEnv(sstore, workspace.DNS)
err = runSSH(sshName, command)
if err != nil {
return breverrors.WrapAndTrace(err)
}
go trackExecAnalytics(sstore, workspaceNameOrID)
return nil
}
err = util.PollUntil(s, workspace.ID, "RUNNING", sstore, " waiting for instance to be ready...", pollTimeout)
if err != nil {
return breverrors.WrapAndTrace(err)

if workspace.Status != "RUNNING" {
return breverrors.WrapAndTrace(fmt.Errorf(
"instance %q is in state %q — please check with: brev ls",
workspaceNameOrID, workspace.Status))
}
refreshRes := refresh.RunRefreshAsync(sstore)

workspace, err = util.GetUserWorkspaceByNameOrIDErr(sstore, workspaceNameOrID)
if err != nil {
// Instance is RUNNING but SSH failed — maybe still booting, do the wait
s := t.NewSpinner()
refreshRes := refresh.RunRefreshAsync(sstore)
if err = refreshRes.Await(); err != nil {
return breverrors.WrapAndTrace(err)
}
if workspace.Status != "RUNNING" {
return breverrors.New("Instance is not running")
}

localIdentifier := workspace.GetLocalIdentifier()
if host {
localIdentifier = workspace.GetHostIdentifier()
}
sshName = string(localIdentifier)

sshName := string(localIdentifier)

err = refreshRes.Await()
if err != nil {
return breverrors.WrapAndTrace(err)
}
err = util.WaitForSSHToBeAvailable(sshName, s)
if err != nil {
return breverrors.WrapAndTrace(err)
return breverrors.WrapAndTrace(fmt.Errorf(
"could not connect to instance %q: %w\nPlease check with: brev ls",
workspaceNameOrID, err))
}
// we don't care about the error here but should log with sentry
// legacy environments wont support this and cause errrors,
// but we don't want to block the user from using the shell
_ = writeconnectionevent.WriteWCEOnEnv(sstore, workspace.DNS)
err = runSSH(sshName, command)
if err != nil {
return breverrors.WrapAndTrace(err)
}
// Call analytics for exec
userID := ""
user, err := sstore.GetCurrentUser()
go trackExecAnalytics(sstore, workspaceNameOrID)
return nil
}

func trackExecAnalytics(sstore ExecStore, workspaceNameOrID string) {
workspace, err := util.GetUserWorkspaceByNameOrIDErr(sstore, workspaceNameOrID)
if err != nil {
userID = workspace.CreatedByUserID
} else {
return
}
userID := workspace.CreatedByUserID
user, err := sstore.GetCurrentUser()
if err == nil {
userID = user.ID
}
data := analytics.EventData{
Expand All @@ -241,19 +290,17 @@ func runExecCommand(t *terminal.Terminal, sstore ExecStore, workspaceNameOrID st
},
}
_ = analytics.TrackEvent(data)

return nil
}

func runSSH(sshAlias string, command string) error {
sshAgentEval := "eval $(ssh-agent -s)"

func runSSHWithTimeout(sshAlias string, command string, connectTimeoutSecs int) error {
// Non-interactive: run command and pipe stdout/stderr
// Escape the command for passing to SSH
escapedCmd := strings.ReplaceAll(command, "'", "'\\''")
cmd := fmt.Sprintf("ssh %s '%s'", sshAlias, escapedCmd)
// -T disables pseudo-terminal allocation (no "Pseudo-terminal will not be allocated" warning)
// Only start ssh-agent if one isn't already running (avoids orphaned agent processes)
agentCmd := `if [ -z "$SSH_AUTH_SOCK" ]; then eval $(ssh-agent -s) > /dev/null; fi`
cmd := fmt.Sprintf("%s && ssh -T -o ConnectTimeout=%d -o LogLevel=ERROR %s '%s'", agentCmd, connectTimeoutSecs, sshAlias, escapedCmd)

cmd = fmt.Sprintf("%s && %s", sshAgentEval, cmd)
sshCmd := exec.Command("bash", "-c", cmd) //nolint:gosec //cmd is user input
sshCmd.Stderr = os.Stderr
sshCmd.Stdout = os.Stdout
Expand All @@ -265,3 +312,7 @@ func runSSH(sshAlias string, command string) error {
}
return nil
}

func runSSH(sshAlias string, command string) error {
return runSSHWithTimeout(sshAlias, command, 10)
}
3 changes: 3 additions & 0 deletions pkg/ssh/ssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ const workspaceSSHConfigTemplate = `Host {{ .Host }}
RequestTTY yes
ForwardAgent yes
AddKeysToAgent yes
ControlMaster auto
ControlPath ~/.ssh/brev-control-%r@%h-%p
ControlPersist 10m
RemoteCommand cd {{ .Dir }}; $SHELL

`
Expand Down
6 changes: 6 additions & 0 deletions pkg/ssh/sshconfigurer.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ const SSHConfigEntryTemplateV2 = `Host {{ .Alias }}
AddKeysToAgent yes
ForwardAgent yes
RequestTTY yes
ControlMaster auto
ControlPath ~/.ssh/brev-control-%r@%h-%p
ControlPersist 10m
{{ if .RunRemoteCMD }}
RemoteCommand cd {{ .Dir }}; $SHELL
{{ end }}
Expand All @@ -250,6 +253,9 @@ const SSHConfigEntryTemplateV3 = `Host {{ .Alias }}
AddKeysToAgent yes
ForwardAgent yes
RequestTTY yes
ControlMaster auto
ControlPath ~/.ssh/brev-control-%r@%h-%p
ControlPersist 10m
Port {{ .Port }}
{{ if .RunRemoteCMD }}
RemoteCommand cd {{ .Dir }}; $SHELL
Expand Down
Loading
Loading