mirror of
https://github.com/safedep/pmg.git
synced 2026-08-03 07:24:09 +02:00
fix(pty): treat background jobs as non-interactive to avoid SIGTTOU stop (#324)
* fix(pty): treat background jobs as non-interactive to avoid SIGTTOU stop IsInteractiveTerminal only checked that stdin/stdout are TTYs. A background job (pmg npm run test &) still has the TTY on stdin/stdout, so pmg picked PTY mode and called tcsetattr to enter raw mode. Changing terminal modes from a background process group makes the kernel stop the process with SIGTTOU, leaving the job hanging in Stopped state. Check that the process group is the terminal's foreground process group (tcgetpgrp == getpgrp) before treating the terminal as interactive, so background jobs fall through to direct execution. Fixes #322 https://claude.ai/code/session_01PBBo5CKkzg68MMGrgCTQcY * test(pty): fail on output copy timeout to avoid racy buffer read Reading the output buffer after a silent select timeout races with the io.Copy goroutine still writing to it. Fail the test on timeout instead. Also fix a grammar nit in the IsInteractiveTerminal doc comment. https://claude.ai/code/session_01PBBo5CKkzg68MMGrgCTQcY --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,21 @@
|
|||||||
|
//go:build !windows
|
||||||
|
|
||||||
|
package pty
|
||||||
|
|
||||||
|
import (
|
||||||
|
"golang.org/x/sys/unix"
|
||||||
|
)
|
||||||
|
|
||||||
|
// isForegroundProcess reports whether this process belongs to the terminal's
|
||||||
|
// foreground process group. A background job (e.g. `pmg npm install &`) still
|
||||||
|
// has the TTY on stdin/stdout, but changing terminal modes or reading from
|
||||||
|
// the TTY in a background process group triggers SIGTTOU/SIGTTIN, which stops
|
||||||
|
// the process.
|
||||||
|
func isForegroundProcess(fd uintptr) bool {
|
||||||
|
foregroundPgrp, err := unix.IoctlGetInt(int(fd), unix.TIOCGPGRP)
|
||||||
|
if err != nil {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
|
return foregroundPgrp == unix.Getpgrp()
|
||||||
|
}
|
||||||
@@ -0,0 +1,9 @@
|
|||||||
|
//go:build windows
|
||||||
|
|
||||||
|
package pty
|
||||||
|
|
||||||
|
// Windows has no Unix-style terminal job control, so a process is never
|
||||||
|
// stopped for touching the console from the "background".
|
||||||
|
func isForegroundProcess(_ uintptr) bool {
|
||||||
|
return true
|
||||||
|
}
|
||||||
@@ -42,9 +42,11 @@ type InteractiveSession interface {
|
|||||||
Close() error
|
Close() error
|
||||||
}
|
}
|
||||||
|
|
||||||
// IsInteractiveTerminal returns true if stdin is a real terminal (TTY).
|
// IsInteractiveTerminal returns true if stdin is a real terminal (TTY) and
|
||||||
// Returns false in CI environments (when the "CI" env var set to "true"),
|
// the process can safely drive it. Returns false in CI environments (when the
|
||||||
// when input or output is piped, or in non-interactive shells.
|
// "CI" env var is set to "true"), when input or output is piped, in
|
||||||
|
// non-interactive shells, or when running as a background job (where putting
|
||||||
|
// the terminal into raw mode would stop the process with SIGTTOU).
|
||||||
func IsInteractiveTerminal() bool {
|
func IsInteractiveTerminal() bool {
|
||||||
if ci := os.Getenv("CI"); ci != "" && strings.ToLower(ci) == "true" {
|
if ci := os.Getenv("CI"); ci != "" && strings.ToLower(ci) == "true" {
|
||||||
return false
|
return false
|
||||||
@@ -58,7 +60,7 @@ func IsInteractiveTerminal() bool {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
return true
|
return isForegroundProcess(os.Stdin.Fd())
|
||||||
}
|
}
|
||||||
|
|
||||||
var _ InteractiveSession = &session{}
|
var _ InteractiveSession = &session{}
|
||||||
|
|||||||
@@ -0,0 +1,113 @@
|
|||||||
|
//go:build !windows
|
||||||
|
|
||||||
|
package pty
|
||||||
|
|
||||||
|
import (
|
||||||
|
"bytes"
|
||||||
|
"context"
|
||||||
|
"fmt"
|
||||||
|
"io"
|
||||||
|
"os"
|
||||||
|
"os/exec"
|
||||||
|
"strings"
|
||||||
|
"syscall"
|
||||||
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
|
"github.com/safedep/ptyx"
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
const ttyHelperEnv = "PMG_TEST_TTY_HELPER"
|
||||||
|
|
||||||
|
// Reproduces https://github.com/safedep/pmg/issues/322: a pmg process started
|
||||||
|
// as a background job (`pmg npm run test &`) still has the TTY on
|
||||||
|
// stdin/stdout, but it is not in the terminal's foreground process group.
|
||||||
|
// Treating it as interactive makes the PTY session call tcsetattr, which
|
||||||
|
// stops the process with SIGTTOU. IsInteractiveTerminal must report false in
|
||||||
|
// that situation.
|
||||||
|
func TestIsInteractiveTerminalBackgroundJob(t *testing.T) {
|
||||||
|
exe, err := os.Executable()
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||||
|
defer cancel()
|
||||||
|
|
||||||
|
// The helper runs on a fresh PTY as the foreground process group, then
|
||||||
|
// moves itself to the background, checking IsInteractiveTerminal in both
|
||||||
|
// states.
|
||||||
|
sess, err := ptyx.Spawn(ctx, ptyx.SpawnOpts{
|
||||||
|
Prog: exe,
|
||||||
|
Args: []string{"-test.run", "^TestIsInteractiveTerminalTTYHelper$", "-test.v"},
|
||||||
|
Env: append(envWithoutCI(), ttyHelperEnv+"=1"),
|
||||||
|
Cols: 80,
|
||||||
|
Rows: 24,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer func() {
|
||||||
|
require.NoError(t, sess.Close())
|
||||||
|
}()
|
||||||
|
|
||||||
|
var output bytes.Buffer
|
||||||
|
copyDone := make(chan struct{})
|
||||||
|
go func() {
|
||||||
|
defer close(copyDone)
|
||||||
|
_, _ = io.Copy(&output, sess.PtyReader())
|
||||||
|
}()
|
||||||
|
|
||||||
|
waitErr := sess.Wait()
|
||||||
|
|
||||||
|
// The buffer must not be read until the copy goroutine is done, both to
|
||||||
|
// avoid a data race and to ensure all helper output has been captured.
|
||||||
|
select {
|
||||||
|
case <-copyDone:
|
||||||
|
case <-time.After(5 * time.Second):
|
||||||
|
t.Fatal("timed out waiting for pty output copy to finish")
|
||||||
|
}
|
||||||
|
|
||||||
|
out := output.String()
|
||||||
|
require.NoError(t, waitErr, "helper failed, output:\n%s", out)
|
||||||
|
assert.Contains(t, out, "foreground_interactive=true", "expected interactive in foreground, output:\n%s", out)
|
||||||
|
assert.Contains(t, out, "background_interactive=false", "expected non-interactive in background, output:\n%s", out)
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestIsInteractiveTerminalTTYHelper is not a real test. It is re-executed by
|
||||||
|
// TestIsInteractiveTerminalBackgroundJob on a PTY where it starts as the
|
||||||
|
// foreground process group.
|
||||||
|
func TestIsInteractiveTerminalTTYHelper(t *testing.T) {
|
||||||
|
if os.Getenv(ttyHelperEnv) != "1" {
|
||||||
|
t.Skip("helper for TestIsInteractiveTerminalBackgroundJob")
|
||||||
|
}
|
||||||
|
|
||||||
|
fmt.Printf("foreground_interactive=%v\n", IsInteractiveTerminal())
|
||||||
|
|
||||||
|
// Hand the terminal's foreground process group to a child, like a shell
|
||||||
|
// does for the foreground job. This process then becomes a background job
|
||||||
|
// on the TTY, matching `pmg npm run test &`.
|
||||||
|
cmd := exec.Command("sleep", "30")
|
||||||
|
cmd.SysProcAttr = &syscall.SysProcAttr{
|
||||||
|
Setpgid: true,
|
||||||
|
Foreground: true,
|
||||||
|
Ctty: int(os.Stdin.Fd()),
|
||||||
|
}
|
||||||
|
require.NoError(t, cmd.Start())
|
||||||
|
defer func() {
|
||||||
|
_ = cmd.Process.Kill()
|
||||||
|
_, _ = cmd.Process.Wait()
|
||||||
|
}()
|
||||||
|
|
||||||
|
fmt.Printf("background_interactive=%v\n", IsInteractiveTerminal())
|
||||||
|
}
|
||||||
|
|
||||||
|
func envWithoutCI() []string {
|
||||||
|
env := make([]string, 0, len(os.Environ()))
|
||||||
|
for _, entry := range os.Environ() {
|
||||||
|
if strings.HasPrefix(entry, "CI=") {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
env = append(env, entry)
|
||||||
|
}
|
||||||
|
|
||||||
|
return env
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user