fix(daemon): resolve the shell in-process and cache successful resolution

apps/daemon/pkg/common/get_shell.go

@@ -5,42 +5,71 @@ package common
 
 import (
 	"os"
-	"os/exec"
 	"strings"
+	"sync"
+)
+
+// shellsFilePath is a package-level var so tests can point resolution at a
+// temporary file.
+var shellsFilePath = "/etc/shells"
 
-	"github.com/daytonaio/daemon/pkg/childreap"
+var (
+	shellCacheMu sync.Mutex
+	cachedShell  string
+	shellCached  bool
 )
 
+// GetShell returns the preferred shell for the sandbox. The first successful
+// resolution (i.e. /etc/shells was readable) is cached for the daemon
+// lifetime; a failed read falls back per-call and is never cached, so a later
+// call retries the file.
 func GetShell() string {
-	cmd := exec.Command("sh", "-c", "grep '^[^#]' /etc/shells")
-	// childreap.Output (not cmd.Output) so the PID-1 reaper winning the
-	// race against cmd.Wait doesn't drop us into the err != nil branch
-	// and silently fall back to "sh" on sandboxes that actually have
-	// zsh/bash available.
-	out, exitCode, err := childreap.Output(cmd)
-	if err != nil || exitCode != 0 {
-		return "sh"
+	shellCacheMu.Lock()
+	defer shellCacheMu.Unlock()
+
+	if shellCached {
+		return cachedShell
 	}
 
-	if strings.Contains(string(out), "/usr/bin/zsh") {
-		return "/usr/bin/zsh"
+	shell, ok := resolveShell(shellsFilePath)
+	if ok {
+		cachedShell = shell
+		shellCached = true
 	}
 
-	if strings.Contains(string(out), "/bin/zsh") {
-		return "/bin/zsh"
+	return shell
+}
+
+// resolveShell reads the shells file at path and picks a shell by preference
+// order: /usr/bin/zsh > /bin/zsh > /usr/bin/bash > /bin/bash > $SHELL (if
+// non-empty) > sh. The boolean reports whether the file was read successfully.
+func resolveShell(path string) (string, bool) {
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return shellFallback(), false
 	}
 
-	if strings.Contains(string(out), "/usr/bin/bash") {
-		return "/usr/bin/bash"
+	var sb strings.Builder
+	for _, line := range strings.Split(string(data), "\n") {
+		if strings.HasPrefix(line, "#") {
+			continue
+		}
+		sb.WriteString(line)
+		sb.WriteByte('\n')
 	}
+	shells := sb.String()
 
-	if strings.Contains(string(out), "/bin/bash") {
-		return "/bin/bash"
+	for _, preferred := range []string{"/usr/bin/zsh", "/bin/zsh", "/usr/bin/bash", "/bin/bash"} {
+		if strings.Contains(shells, preferred) {
+			return preferred, true
+		}
 	}
 
-	shellEnv, shellSet := os.LookupEnv("SHELL")
+	return shellFallback(), true
+}
 
-	if shellSet {
+func shellFallback() string {
+	if shellEnv := os.Getenv("SHELL"); shellEnv != "" {
 		return shellEnv
 	}
 

apps/daemon/pkg/common/get_shell_test.go

@@ -0,0 +1,162 @@
+// Copyright Daytona Platforms Inc.
+// SPDX-License-Identifier: AGPL-3.0
+
+package common
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func writeShellsFile(t *testing.T, content string) string {
+	t.Helper()
+	path := filepath.Join(t.TempDir(), "shells")
+	if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	return path
+}
+
+// unsetShellEnv unsets $SHELL for the test, restoring the original value
+// (or unset state) afterwards via t.Setenv's cleanup.
+func unsetShellEnv(t *testing.T) {
+	t.Helper()
+	t.Setenv("SHELL", "")
+	if err := os.Unsetenv("SHELL"); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func resetShellCache(t *testing.T) {
+	t.Helper()
+	reset := func() {
+		shellCacheMu.Lock()
+		defer shellCacheMu.Unlock()
+		cachedShell = ""
+		shellCached = false
+	}
+	reset()
+	t.Cleanup(reset)
+}
+
+func swapShellsFilePath(t *testing.T, path string) {
+	t.Helper()
+	original := shellsFilePath
+	shellsFilePath = path
+	t.Cleanup(func() { shellsFilePath = original })
+}
+
+func TestResolveShellPrefersZshOverBash(t *testing.T) {
+	path := writeShellsFile(t, "/bin/sh\n/bin/bash\n/usr/bin/bash\n/bin/zsh\n/usr/bin/zsh\n")
+
+	shell, ok := resolveShell(path)
+	if !ok {
+		t.Fatal("expected successful resolution")
+	}
+	if shell != "/usr/bin/zsh" {
+		t.Fatalf("expected /usr/bin/zsh, got %q", shell)
+	}
+}
+
+func TestResolveShellBashWhenNoZsh(t *testing.T) {
+	path := writeShellsFile(t, "/bin/sh\n/bin/bash\n")
+
+	shell, ok := resolveShell(path)
+	if !ok {
+		t.Fatal("expected successful resolution")
+	}
+	if shell != "/bin/bash" {
+		t.Fatalf("expected /bin/bash, got %q", shell)
+	}
+}
+
+func TestResolveShellEnvFallbackWhenNoPreferredShell(t *testing.T) {
+	t.Setenv("SHELL", "/opt/custom/fish")
+	path := writeShellsFile(t, "/bin/sh\n/opt/custom/fish\n")
+
+	shell, ok := resolveShell(path)
+	if !ok {
+		t.Fatal("expected successful resolution")
+	}
+	if shell != "/opt/custom/fish" {
+		t.Fatalf("expected /opt/custom/fish, got %q", shell)
+	}
+}
+
+func TestResolveShellShWhenFileMissingAndNoShellEnv(t *testing.T) {
+	unsetShellEnv(t)
+	path := filepath.Join(t.TempDir(), "does-not-exist")
+
+	shell, ok := resolveShell(path)
+	if ok {
+		t.Fatal("expected failed resolution for missing file")
+	}
+	if shell != "sh" {
+		t.Fatalf("expected sh, got %q", shell)
+	}
+}
+
+func TestResolveShellShWhenFileMissingAndEmptyShellEnv(t *testing.T) {
+	t.Setenv("SHELL", "")
+	path := filepath.Join(t.TempDir(), "does-not-exist")
+
+	shell, ok := resolveShell(path)
+	if ok {
+		t.Fatal("expected failed resolution for missing file")
+	}
+	if shell != "sh" {
+		t.Fatalf("expected sh, got %q", shell)
+	}
+}
+
+func TestResolveShellIgnoresCommentLines(t *testing.T) {
+	unsetShellEnv(t)
+	path := writeShellsFile(t, "# /usr/bin/zsh\n#/bin/zsh\n/bin/bash\n")
+
+	shell, ok := resolveShell(path)
+	if !ok {
+		t.Fatal("expected successful resolution")
+	}
+	if shell != "/bin/bash" {
+		t.Fatalf("expected /bin/bash, got %q", shell)
+	}
+}
+
+func TestGetShellCachesSuccessfulResolution(t *testing.T) {
+	resetShellCache(t)
+	path := writeShellsFile(t, "/usr/bin/zsh\n")
+	swapShellsFilePath(t, path)
+
+	if shell := GetShell(); shell != "/usr/bin/zsh" {
+		t.Fatalf("expected /usr/bin/zsh, got %q", shell)
+	}
+
+	// Mutate the file; the cached answer must not change.
+	if err := os.WriteFile(path, []byte("/bin/bash\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if shell := GetShell(); shell != "/usr/bin/zsh" {
+		t.Fatalf("expected cached /usr/bin/zsh, got %q", shell)
+	}
+}
+
+func TestGetShellDoesNotCacheFailedResolution(t *testing.T) {
+	resetShellCache(t)
+	unsetShellEnv(t)
+	path := filepath.Join(t.TempDir(), "shells")
+	swapShellsFilePath(t, path)
+
+	if shell := GetShell(); shell != "sh" {
+		t.Fatalf("expected sh while file is missing, got %q", shell)
+	}
+
+	// The failed read must not have been cached: once the file appears,
+	// the next call picks it up.
+	if err := os.WriteFile(path, []byte("/bin/bash\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if shell := GetShell(); shell != "/bin/bash" {
+		t.Fatalf("expected /bin/bash after file created, got %q", shell)
+	}
+}