Skip to content

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

Open
mu-hashmi wants to merge 2 commits into
mainfrom
fix/daemon-shell-resolution
Open

fix(daemon): resolve the shell in-process and cache successful resolution#5003
mu-hashmi wants to merge 2 commits into
mainfrom
fix/daemon-shell-resolution

Conversation

@mu-hashmi

@mu-hashmi mu-hashmi commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

TL;DR: the daemon resolves the shell by reading /etc/shells in-process (no subprocess), caches a successful resolution for its lifetime, and can never return an empty shell.

Description

Every toolbox exec, code-run, session create, and PTY spawn resolved the shell by spawning sh -c \"grep '^[^#]' /etc/shells\" - an extra fork+execve on the daemon's five hottest call sites, and the first thing to fail under degraded filesystem conditions (during the fd-exhaustion incident the probe died before the real command, silently downgrading users to sh).

Now GetShell():

  • reads /etc/shells directly and preserves the exact preference order (/usr/bin/zsh > /bin/zsh > /usr/bin/bash > /bin/bash > $SHELL if non-empty > sh);
  • caches a successful resolution behind a mutex (daemon serves concurrent requests);
  • never caches failure - an unreadable file falls back for that call only ($SHELL, then sh) and retries next call;
  • never returns empty: $SHELL set-but-empty falls through to sh (review-caught edge; regression-tested).

Behavior note: a shell installed after daemon start is not picked up until restart (previously each exec re-probed). Accepted: daemon lifetime == sandbox session.

Review guide

  1. apps/daemon/pkg/common/get_shell.go - the whole change.
  2. get_shell_test.go - 8 cases document the contract.

Lowest-risk PR of the six; no generated files, no API surface.

Commit map

Commit What Why it exists
c9ee8ef in-process resolution + success-only cache implementation (plan)
dac41ca never return empty when /etc/shells unreadable cubic review (valid)

Related PRs

Independent. Siblings from the same incident: #5001, #5002.

Validation

  • 8 unit tests: preference order, comment filtering, $SHELL fallback, missing-file fallback, empty-$SHELL regression, success cached (file mutated after first call - answer unchanged), failure not cached (file appears later - next call picks it up). Runs on darwin and in CI.
  • gofmt clean, golangci-lint v2.6.2 zero issues.
  • All CI checks green on dac41ca.

Closes #4997

Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
@nx-cloud

nx-cloud Bot commented Jun 11, 2026

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit dac41ca

Command Status Duration Result
nx e2e:cleanup daytona-e2e ✅ Succeeded <1s View ↗
nx run-many --target=test:e2e --all --nxBail=true ✅ Succeeded 6m 39s View ↗
nx e2e daytona-e2e ✅ Succeeded 55s View ↗
nx run-many --target=build --projects=api,runne... ✅ Succeeded 18s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-06-11 04:10:25 UTC

@mu-hashmi mu-hashmi marked this pull request as ready for review June 11, 2026 03:32
Copilot AI review requested due to automatic review settings June 11, 2026 03:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the daemon’s shell selection logic to avoid spawning a subprocess on hot paths by reading /etc/shells in-process and caching the first successful resolution for the daemon lifetime (while not caching failures to allow later retries).

Changes:

  • Replaced sh -c "grep ... /etc/shells" probing with in-process /etc/shells parsing and preference-based selection.
  • Added mutex-guarded caching so successful shell resolution is computed once and reused across requests.
  • Added unit tests covering preference order, comment filtering, env/missing-file fallback, and caching semantics.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
apps/daemon/pkg/common/get_shell.go In-process /etc/shells parsing, fallback behavior, and mutex-guarded cache for resolved shell.
apps/daemon/pkg/common/get_shell_test.go New unit tests validating shell resolution order, comment filtering, and caching/retry behavior.
Comments suppressed due to low confidence (1)

apps/daemon/pkg/common/get_shell.go:77

  • os.LookupEnv("SHELL") returns shellSet=true even when the value is an empty string. In that case shellFallback() currently returns "", which can later cause exec.Command(shell) (e.g. in SpawnTTY) to fail. Treat empty $SHELL as unset and fall back to sh.
func shellFallback() string {
	if shellEnv, shellSet := os.LookupEnv("SHELL"); shellSet {
		return shellEnv
	}

	return "sh"
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/daemon/pkg/common/get_shell.go Outdated
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

daemon: resolve the shell once instead of probing /etc/shells on every exec

2 participants