diff --git a/daemon/AGENTS.md b/daemon/AGENTS.md index 716a73d..8c3f0ca 100644 --- a/daemon/AGENTS.md +++ b/daemon/AGENTS.md @@ -23,9 +23,11 @@ databases, command parsing, and shutdown policy belong to the caller. process checks, or permissions differ. - Auto-start goes through the caller-provided `StartFunc`; this package must not invent application launch commands. -- Windows detached children run on a hidden console (`CREATE_NO_WINDOW`), never - `DETACHED_PROCESS`. A console-less child makes every console-subsystem - descendant (git, taskkill, agent CLIs) open a visible console window. +- Windows detached children use `DETACHED_PROCESS`, not `CREATE_NO_WINDOW`. + Hidden consoles expose `CONIN$`, which can make terminal-probing libraries + block forever at daemon startup. Non-interactive console-subsystem + descendants that must avoid visible windows should set `CREATE_NO_WINDOW` at + their own spawn site. ## Tests diff --git a/daemon/detach_windows.go b/daemon/detach_windows.go index 94cdf69..1d70828 100644 --- a/daemon/detach_windows.go +++ b/daemon/detach_windows.go @@ -7,18 +7,14 @@ import ( "syscall" ) -// createNoWindow (CREATE_NO_WINDOW) runs the child on a hidden console -// instead of no console. A DETACHED_PROCESS child has no console, so every -// console-subsystem descendant it spawns (git, taskkill, agent CLIs, ...) -// allocates a new visible console window. A hidden console is inherited by -// descendants, so background daemons never flash terminal windows. The two -// flags are mutually exclusive: CREATE_NO_WINDOW is ignored when -// DETACHED_PROCESS is set. -const createNoWindow = 0x08000000 +// windowsDetachedProcess (DETACHED_PROCESS) starts the daemon without a console. +// Hidden consoles expose CONIN$, which can make terminal-probing libraries block +// forever when nothing is present to answer terminal queries. +const windowsDetachedProcess = 0x00000008 func detachChild(cmd *exec.Cmd) { if cmd.SysProcAttr == nil { cmd.SysProcAttr = &syscall.SysProcAttr{} } - cmd.SysProcAttr.CreationFlags |= syscall.CREATE_NEW_PROCESS_GROUP | createNoWindow + cmd.SysProcAttr.CreationFlags |= syscall.CREATE_NEW_PROCESS_GROUP | windowsDetachedProcess } diff --git a/daemon/detach_windows_test.go b/daemon/detach_windows_test.go index a1614a5..70894db 100644 --- a/daemon/detach_windows_test.go +++ b/daemon/detach_windows_test.go @@ -10,12 +10,12 @@ import ( "github.com/stretchr/testify/assert" ) -// detachedProcess (DETACHED_PROCESS) must never come back: it is mutually -// exclusive with CREATE_NO_WINDOW, and a child with no console makes every -// console-subsystem descendant open a visible console window. -const detachedProcess = 0x00000008 +const ( + detachedProcess = 0x00000008 + createNoWindow = 0x08000000 +) -func TestDetachChildUsesHiddenConsole(t *testing.T) { +func TestDetachChildDetachesWithoutConsole(t *testing.T) { assert := assert.New(t) cmd := exec.Command("cmd.exe") @@ -23,8 +23,8 @@ func TestDetachChildUsesHiddenConsole(t *testing.T) { flags := cmd.SysProcAttr.CreationFlags assert.NotZero(flags&syscall.CREATE_NEW_PROCESS_GROUP, "child must run in its own process group") - assert.NotZero(flags&createNoWindow, "child must run on a hidden console") - assert.Zero(flags&detachedProcess, "DETACHED_PROCESS would make console descendants open visible windows") + assert.NotZero(flags&detachedProcess, "child must start without an attached console") + assert.Zero(flags&createNoWindow, "hidden consoles expose CONIN$ and can block terminal probes") } func TestDetachChildPreservesExistingSysProcAttr(t *testing.T) { @@ -37,5 +37,5 @@ func TestDetachChildPreservesExistingSysProcAttr(t *testing.T) { flags := cmd.SysProcAttr.CreationFlags assert.NotZero(flags&createSuspended, "caller-set creation flags must be preserved") - assert.NotZero(flags & createNoWindow) + assert.NotZero(flags & detachedProcess) } diff --git a/daemon/start.go b/daemon/start.go index ed58d4d..b0887f9 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -23,9 +23,8 @@ type StartDetachedOptions struct { } // StartDetached starts a child process detached from the caller's process -// group where the platform supports it. On Windows the child runs on its own -// hidden console so neither it nor its console-subsystem descendants open -// visible console windows. +// group where the platform supports it. On Windows the child starts without a +// console so terminal probes cannot block waiting for console input. func StartDetached(ctx context.Context, opts StartDetachedOptions) error { if err := ctx.Err(); err != nil { return err diff --git a/daemon/start_detached_windows_test.go b/daemon/start_detached_windows_test.go new file mode 100644 index 0000000..cc39263 --- /dev/null +++ b/daemon/start_detached_windows_test.go @@ -0,0 +1,62 @@ +//go:build windows + +package daemon_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.kenn.io/kit/daemon" +) + +// TestStartDetachedConsoleInputHelper is not a real test: +// TestStartDetachedDoesNotExposeConsoleInput re-executes the test binary with +// -test.run targeting it so the detached child can report whether CONIN$ +// exists in its own process. +func TestStartDetachedConsoleInputHelper(t *testing.T) { + marker := os.Getenv("KIT_DAEMON_TEST_CONSOLE_MARKER") + if marker == "" { + t.Skip("helper process for TestStartDetachedDoesNotExposeConsoleInput") + } + file, err := os.OpenFile("CONIN$", os.O_RDONLY, 0) + if err == nil { + _ = file.Close() + require.NoError(t, os.WriteFile(marker, []byte("console-input-present"), 0o600)) + return + } + require.NoError(t, os.WriteFile(marker, []byte("console-input-unavailable"), 0o600)) +} + +func TestStartDetachedDoesNotExposeConsoleInput(t *testing.T) { + exe, err := os.Executable() + require.NoError(t, err) + marker := filepath.Join(t.TempDir(), "console-marker") + + err = daemon.StartDetached(context.Background(), daemon.StartDetachedOptions{ + Executable: exe, + Args: []string{"-test.run", "^TestStartDetachedConsoleInputHelper$"}, + Env: append(os.Environ(), "KIT_DAEMON_TEST_CONSOLE_MARKER="+marker), + }) + require.NoError(t, err) + + got := waitForMarker(t, marker) + require.Equal(t, "console-input-unavailable", strings.TrimSpace(got)) +} + +func waitForMarker(t *testing.T, marker string) string { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for { + data, err := os.ReadFile(marker) + if err == nil { + return string(data) + } + require.True(t, time.Now().Before(deadline), "detached child never wrote marker file") + time.Sleep(25 * time.Millisecond) + } +} diff --git a/git/cmd/gitcmd.go b/git/cmd/gitcmd.go index c3e1f45..0a60795 100644 --- a/git/cmd/gitcmd.go +++ b/git/cmd/gitcmd.go @@ -93,7 +93,7 @@ func (r Runner) Command(ctx context.Context, dir string, args ...string) *exec.C if r.basicAuth != nil { panic("gitcmd: Command cannot be used with WithBasicAuth; use Run or Output so credentials can be cleaned up") } - cmd := gitCommand(ctx, args...) + cmd := gitCommand(ctx, !r.TerminalPrompt, args...) cmd.Dir = dir cmd.Env, _ = r.commandEnv(ctx, dir) return cmd @@ -107,7 +107,7 @@ func (r Runner) Output(ctx context.Context, dir string, args ...string) ([]byte, // Run runs git and returns stdout, stderr, and a *GitError on failure. func (r Runner) Run(ctx context.Context, dir string, stdin io.Reader, args ...string) ([]byte, []byte, error) { - cmd := gitCommand(ctx, args...) + cmd := gitCommand(ctx, !r.TerminalPrompt, args...) cmd.Dir = dir var cleanup func() cmd.Env, cleanup = r.commandEnv(ctx, dir) @@ -131,9 +131,9 @@ func (r Runner) Run(ctx context.Context, dir string, stdin io.Reader, args ...st return stdout.Bytes(), stderr.Bytes(), nil } -func gitCommand(ctx context.Context, args ...string) *exec.Cmd { +func gitCommand(ctx context.Context, hideConsoleWindow bool, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, "git", args...) - prepareGitCommand(cmd) + prepareGitCommand(cmd, hideConsoleWindow) return cmd } @@ -237,7 +237,7 @@ func readSafeDirectories(ctx context.Context, env []string, dir string) []string for _, scope := range scopes { // --includes is required for explicit-scope reads to honor include.path // and includeIf directives the way git's default config sequence does. - cmd := gitCommand(ctx, "config", scope, "--includes", "-z", "--get-all", "safe.directory") + cmd := gitCommand(ctx, true, "config", scope, "--includes", "-z", "--get-all", "safe.directory") cmd.Dir = dir cmd.Env = env out, err := cmd.Output() diff --git a/git/cmd/gitcmd_other.go b/git/cmd/gitcmd_other.go index 2c85d29..1c620de 100644 --- a/git/cmd/gitcmd_other.go +++ b/git/cmd/gitcmd_other.go @@ -4,4 +4,4 @@ package gitcmd import "os/exec" -func prepareGitCommand(cmd *exec.Cmd) {} +func prepareGitCommand(*exec.Cmd, bool) {} diff --git a/git/cmd/gitcmd_windows.go b/git/cmd/gitcmd_windows.go index 2b326e4..fa81b33 100644 --- a/git/cmd/gitcmd_windows.go +++ b/git/cmd/gitcmd_windows.go @@ -9,7 +9,10 @@ import ( "golang.org/x/sys/windows" ) -func prepareGitCommand(cmd *exec.Cmd) { +func prepareGitCommand(cmd *exec.Cmd, hideConsoleWindow bool) { + if !hideConsoleWindow { + return + } if cmd.SysProcAttr == nil { cmd.SysProcAttr = &syscall.SysProcAttr{} } diff --git a/git/cmd/gitcmd_windows_test.go b/git/cmd/gitcmd_windows_test.go index 1d7810b..fb2fe32 100644 --- a/git/cmd/gitcmd_windows_test.go +++ b/git/cmd/gitcmd_windows_test.go @@ -6,16 +6,25 @@ import ( "context" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/sys/windows" ) -func TestRunnerCommandHidesGitConsoleWindowOnWindows(t *testing.T) { +func TestRunnerCommandHidesGitConsoleWindow(t *testing.T) { cmd := New().Command(context.Background(), "", "status") - if cmd.SysProcAttr == nil { - t.Fatal("git command SysProcAttr is nil") - } - if cmd.SysProcAttr.CreationFlags&windows.CREATE_NO_WINDOW == 0 { - t.Fatalf("git command creation flags = %#x, want CREATE_NO_WINDOW", cmd.SysProcAttr.CreationFlags) + require.NotNil(t, cmd.SysProcAttr) + assert.NotZero(t, cmd.SysProcAttr.CreationFlags&windows.CREATE_NO_WINDOW, "git subprocesses must not flash console windows") +} + +func TestRunnerCommandAllowsConsoleWindowForTerminalPrompts(t *testing.T) { + runner := New() + runner.TerminalPrompt = true + + cmd := runner.Command(context.Background(), "", "fetch") + + if cmd.SysProcAttr != nil { + assert.Zero(t, cmd.SysProcAttr.CreationFlags&windows.CREATE_NO_WINDOW, "interactive git prompts should be able to use the console") } }