Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single Session Mode #1634

Closed
wants to merge 5 commits into from
Closed

Single Session Mode #1634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d9bac1c
feat: first pass at a single session command
oneirocosm Dec 27, 2024
7880eb8
feat: add blockcontroller outline for single shell
oneirocosm Dec 27, 2024
f86b607
Merge branch 'main' into sylvie/single-session
oneirocosm Jan 2, 2025
1db0a60
merge branch 'main' into sylvie/single-session
oneirocosm Jan 2, 2025
ce7d6dd
fix: change return type in singleSession code
oneirocosm Jan 3, 2025
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
1 change: 1 addition & 0 deletions frontend/types/gotypes.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ declare global {
"conn:wshenabled"?: boolean;
"conn:askbeforewshinstall"?: boolean;
"conn:overrideconfig"?: boolean;
"conn:singlesession"?: boolean;
"display:hidden"?: boolean;
"display:order"?: number;
"term:*"?: boolean;
Expand Down
37 changes: 37 additions & 0 deletions pkg/blockcontroller/blockcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,13 @@ func (bc *BlockController) setupAndStartShellProcess(rc *RunShellOpts, blockMeta
} else {
return nil, fmt.Errorf("unknown controller type %q", bc.ControllerType)
}

var singleSession bool
fullConfig := wconfig.ReadFullConfig()
existingConnection, ok := fullConfig.Connections[remoteName]
if ok {
singleSession = existingConnection.ConnSingleSession
}
var shellProc *shellexec.ShellProc
if strings.HasPrefix(remoteName, "wsl://") {
wslName := strings.TrimPrefix(remoteName, "wsl://")
Expand All @@ -347,6 +354,36 @@ func (bc *BlockController) setupAndStartShellProcess(rc *RunShellOpts, blockMeta
if err != nil {
return nil, err
}
} else if remoteName != "" && singleSession {
credentialCtx, cancelFunc := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelFunc()

opts, err := remote.ParseOpts(remoteName)
if err != nil {
return nil, err
}
conn := conncontroller.GetConn(credentialCtx, opts, false, &wshrpc.ConnKeywords{})
connStatus := conn.DeriveConnStatus()
if connStatus.Status != conncontroller.Status_Connected {
return nil, fmt.Errorf("not connected, cannot start shellproc")
}
if !blockMeta.GetBool(waveobj.MetaKey_CmdNoWsh, false) {
jwtStr, err := wshutil.MakeClientJWTToken(wshrpc.RpcContext{TabId: bc.TabId, BlockId: bc.BlockId, Conn: conn.Opts.String()}, conn.GetDomainSocketName())
if err != nil {
return nil, fmt.Errorf("error making jwt token: %w", err)
}
cmdOpts.Env[wshutil.WaveJwtTokenVarName] = jwtStr
}
shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn)
if err != nil {
return nil, err
}
// todo
// i have disabled the conn server for this type of connection
// this means we need to receive a signal from the process once it has
// opened a domain socket. then, once that is done, we can forward the
// unix domain socket here. also, we need to set the wsh boolean true as
// is done in the current connserver implementation
Comment on lines +357 to +386
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for domain socket setup.

The current implementation lacks proper error handling for the case where StartSingleSessionRemoteShellProc succeeds but the subsequent domain socket setup fails. This could lead to resource leaks or hanging connections.

Consider adding cleanup code and proper error propagation:

 		shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn)
 		if err != nil {
 			return nil, err
 		}
+		// Wait for domain socket setup with timeout
+		socketSetupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		select {
+		case <-socketSetupCtx.Done():
+			shellProc.Close()
+			return nil, fmt.Errorf("timeout waiting for domain socket setup")
+		case err := <-shellProc.SocketSetupErrCh:
+			if err != nil {
+				shellProc.Close()
+				return nil, fmt.Errorf("domain socket setup failed: %w", err)
+			}
+		}

Committable suggestion skipped: line range outside the PR's diff.

} else if remoteName != "" {
credentialCtx, cancelFunc := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelFunc()
Expand Down
4 changes: 3 additions & 1 deletion pkg/remote/conncontroller/conncontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ func (conn *SSHConn) connectInternal(ctx context.Context, connFlags *wshrpc.Conn
})
config := wconfig.ReadFullConfig()
enableWsh := config.Settings.ConnWshEnabled
var singleSession bool
askBeforeInstall := config.Settings.ConnAskBeforeWshInstall
connSettings, ok := config.Connections[conn.GetName()]
if ok {
Expand All @@ -521,8 +522,9 @@ func (conn *SSHConn) connectInternal(ctx context.Context, connFlags *wshrpc.Conn
if connSettings.ConnAskBeforeWshInstall != nil {
askBeforeInstall = *connSettings.ConnAskBeforeWshInstall
}
singleSession = connSettings.ConnSingleSession
}
if enableWsh {
if enableWsh && !singleSession {
installErr := conn.CheckAndInstallWsh(ctx, clientDisplayName, &WshInstallOpts{NoUserPrompt: !askBeforeInstall})
if errors.Is(installErr, &WshInstallSkipError{}) {
// skips are not true errors
Expand Down
72 changes: 72 additions & 0 deletions pkg/shellexec/shellexec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"bytes"
"context"
"fmt"
"html/template"
"io"
"log"
"os"
Expand Down Expand Up @@ -241,6 +242,77 @@ func StartWslShellProc(ctx context.Context, termSize waveobj.TermSize, cmdStr st
return &ShellProc{Cmd: cmdWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil
}

var singleSessionCmdTemplate = `bash -c ' \
{{.wshPath}} connserver --single || \
( uname -m && \
read -r count &&
for ((i=0; i<count; i++)); do \
read -r line || break; \
printf "%%b" "$line"; \
done > {{.tempPath}} && \
chmod a+x {{.tempPath}} && \
mv {{.tempPath}} {{.wshPath}} &&
{{.wshPath}} connserver --single \
)
`
Comment on lines +245 to +257
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and secure file permissions.

The command template has several potential issues:

  1. No error handling for file operations (read/write/chmod/mv)
  2. Using chmod a+x grants execute permission to all users, which is overly permissive
  3. No cleanup of temporary file on failure

Consider these improvements:

var singleSessionCmdTemplate = `bash -c ' \
{{.wshPath}} connserver --single || \
( uname -m && \
  read -r count &&
  for ((i=0; i<count; i++)); do \
      read -r line || break; \
      printf "%%b" "$line"; \
  done > {{.tempPath}} && \
- chmod a+x {{.tempPath}} && \
+ chmod 0700 {{.tempPath}} && \
  mv {{.tempPath}} {{.wshPath}} && 
  {{.wshPath}} connserver --single \
) || { rm -f {{.tempPath}}; exit 1; }
'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var singleSessionCmdTemplate = `bash -c ' \
{{.wshPath}} connserver --single || \
( uname -m && \
read -r count &&
for ((i=0; i<count; i++)); do \
read -r line || break; \
printf "%%b" "$line"; \
done > {{.tempPath}} && \
chmod a+x {{.tempPath}} && \
mv {{.tempPath}} {{.wshPath}} &&
{{.wshPath}} connserver --single \
)
`
var singleSessionCmdTemplate = `bash -c ' \
{{.wshPath}} connserver --single || \
( uname -m && \
read -r count &&
for ((i=0; i<count; i++)); do \
read -r line || break; \
printf "%%b" "$line"; \
done > {{.tempPath}} && \
chmod 0700 {{.tempPath}} && \
mv {{.tempPath}} {{.wshPath}} &&
{{.wshPath}} connserver --single \
) || { rm -f {{.tempPath}}; exit 1; }
'


func StartSingleSessionRemoteShellProc(termSize waveobj.TermSize, conn *conncontroller.SSHConn) (*ShellProc, error) {
client := conn.GetClient()
session, err := client.NewSession()
if err != nil {
return nil, err
}
remoteStdinRead, remoteStdinWriteOurs, err := os.Pipe()
if err != nil {
return nil, err
}

remoteStdoutReadOurs, remoteStdoutWrite, err := os.Pipe()
if err != nil {
return nil, err
}

pipePty := &PipePty{
remoteStdinWrite: remoteStdinWriteOurs,
remoteStdoutRead: remoteStdoutReadOurs,
}
if termSize.Rows == 0 || termSize.Cols == 0 {
termSize.Rows = shellutil.DefaultTermRows
termSize.Cols = shellutil.DefaultTermCols
}
if termSize.Rows <= 0 || termSize.Cols <= 0 {
return nil, fmt.Errorf("invalid term size: %v", termSize)
}
session.Stdin = remoteStdinRead
session.Stdout = remoteStdoutWrite
session.Stderr = remoteStdoutWrite

session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil)

wshDir := "~/.waveterm/bin"
wshPath := wshDir + "/wsh"
var installWords = map[string]string{
"wshPath": wshPath,
"tempPath": wshPath + ".temp",
}
Comment on lines +292 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate directory existence before file operations.

The code assumes the ~/.waveterm/bin directory exists without validation.

Add directory validation and creation:

wshDir := "~/.waveterm/bin"
+if err := os.MkdirAll(wshDir, 0700); err != nil {
+    return nil, fmt.Errorf("failed to create wsh directory: %w", err)
+}
wshPath := wshDir + "/wsh"

Committable suggestion skipped: line range outside the PR's diff.


//todo add code that allows streaming base64 for download

singleSessionCmd := &bytes.Buffer{}
cmdTemplate := template.Must(template.New("").Parse(singleSessionCmdTemplate))
cmdTemplate.Execute(singleSessionCmd, installWords)

log.Printf("full single session command is: %s", singleSessionCmd)
sessionWrap := MakeSessionWrap(session, singleSessionCmd.String(), pipePty)

err = sessionWrap.Start()
if err != nil {
pipePty.Close()
return nil, err
}
return &ShellProc{Cmd: sessionWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil
}
Comment on lines +259 to +314
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check the error from session.RequestPty
The call to session.RequestPty might fail, leaving the session in a partial state. Handling this error helps identify pty allocation issues.

- session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil)
+if errPty := session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil); errPty != nil {
+    pipePty.Close()
+    session.Close()
+    return nil, fmt.Errorf("failed to request pty: %w", errPty)
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func StartSingleSessionRemoteShellProc(termSize waveobj.TermSize, conn *conncontroller.SSHConn) (*ShellProc, error) {
client := conn.GetClient()
session, err := client.NewSession()
if err != nil {
return nil, err
}
remoteStdinRead, remoteStdinWriteOurs, err := os.Pipe()
if err != nil {
return nil, err
}
remoteStdoutReadOurs, remoteStdoutWrite, err := os.Pipe()
if err != nil {
return nil, err
}
pipePty := &PipePty{
remoteStdinWrite: remoteStdinWriteOurs,
remoteStdoutRead: remoteStdoutReadOurs,
}
if termSize.Rows == 0 || termSize.Cols == 0 {
termSize.Rows = shellutil.DefaultTermRows
termSize.Cols = shellutil.DefaultTermCols
}
if termSize.Rows <= 0 || termSize.Cols <= 0 {
return nil, fmt.Errorf("invalid term size: %v", termSize)
}
session.Stdin = remoteStdinRead
session.Stdout = remoteStdoutWrite
session.Stderr = remoteStdoutWrite
session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil)
wshDir := "~/.waveterm/bin"
wshPath := wshDir + "/wsh"
var installWords = map[string]string{
"wshPath": wshPath,
"tempPath": wshPath + ".temp",
}
//todo add code that allows streaming base64 for download
singleSessionCmd := &bytes.Buffer{}
cmdTemplate := template.Must(template.New("").Parse(singleSessionCmdTemplate))
cmdTemplate.Execute(singleSessionCmd, installWords)
log.Printf("full single session command is: %s", singleSessionCmd)
sessionWrap := MakeSessionWrap(session, singleSessionCmd.String(), pipePty)
err = sessionWrap.Start()
if err != nil {
pipePty.Close()
return nil, err
}
return &ShellProc{Cmd: sessionWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil
}
func StartSingleSessionRemoteShellProc(termSize waveobj.TermSize, conn *conncontroller.SSHConn) (*ShellProc, error) {
client := conn.GetClient()
session, err := client.NewSession()
if err != nil {
return nil, err
}
remoteStdinRead, remoteStdinWriteOurs, err := os.Pipe()
if err != nil {
return nil, err
}
remoteStdoutReadOurs, remoteStdoutWrite, err := os.Pipe()
if err != nil {
return nil, err
}
pipePty := &PipePty{
remoteStdinWrite: remoteStdinWriteOurs,
remoteStdoutRead: remoteStdoutReadOurs,
}
if termSize.Rows == 0 || termSize.Cols == 0 {
termSize.Rows = shellutil.DefaultTermRows
termSize.Cols = shellutil.DefaultTermCols
}
if termSize.Rows <= 0 || termSize.Cols <= 0 {
return nil, fmt.Errorf("invalid term size: %v", termSize)
}
session.Stdin = remoteStdinRead
session.Stdout = remoteStdoutWrite
session.Stderr = remoteStdoutWrite
if errPty := session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil); errPty != nil {
pipePty.Close()
session.Close()
return nil, fmt.Errorf("failed to request pty: %w", errPty)
}
wshDir := "~/.waveterm/bin"
wshPath := wshDir + "/wsh"
var installWords = map[string]string{
"wshPath": wshPath,
"tempPath": wshPath + ".temp",
}
//todo add code that allows streaming base64 for download
singleSessionCmd := &bytes.Buffer{}
cmdTemplate := template.Must(template.New("").Parse(singleSessionCmdTemplate))
cmdTemplate.Execute(singleSessionCmd, installWords)
log.Printf("full single session command is: %s", singleSessionCmd)
sessionWrap := MakeSessionWrap(session, singleSessionCmd.String(), pipePty)
err = sessionWrap.Start()
if err != nil {
pipePty.Close()
return nil, err
}
return &ShellProc{Cmd: sessionWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil
}


func StartRemoteShellProcNoWsh(termSize waveobj.TermSize, cmdStr string, cmdOpts CommandOptsType, conn *conncontroller.SSHConn) (*ShellProc, error) {
client := conn.GetClient()
session, err := client.NewSession()
Expand Down
1 change: 1 addition & 0 deletions pkg/wshrpc/wshrpctypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ type ConnKeywords struct {
ConnWshEnabled *bool `json:"conn:wshenabled,omitempty"`
ConnAskBeforeWshInstall *bool `json:"conn:askbeforewshinstall,omitempty"`
ConnOverrideConfig bool `json:"conn:overrideconfig,omitempty"`
ConnSingleSession bool `json:"conn:singlesession,omitempty"`

DisplayHidden *bool `json:"display:hidden,omitempty"`
DisplayOrder float32 `json:"display:order,omitempty"`
Expand Down
Loading