feat: handle Ctrl-Z / fg transparently for wrapped commands

[voidexpr]

This is all done by claude code but seems reasonable. I did some simple manual testing. I really appreciate your work with fence, I'm just starting to use it but I'm hoping to use it a lot more so hopefully this patch can help. The use case motivating this: fence around claude code and trying to put it in the background to type a few commands before going back to claude code.

When fence wrapped an interactive command on a real TTY, pressing Ctrl-Z left the child stopped while fence kept blocking in Wait(); the user's outer shell never printed "[1]+ Stopped" and the session felt hung.

Add a stop-aware wait4 loop (jobcontrol.go) that runs whenever fence is managing the host TTY foreground. On a child stop it hands the terminal back to fence's own pgrp, SIGSTOPs fence so the outer shell prints the familiar Stopped line, and on resume re-grants the TTY to the child and SIGCONTs the child group.

Notably, the loop deliberately does NOT pass WCONTINUED to wait4. On Darwin the kernel reports a continued child with status 0x137F — the standard 0x7F "stopped" low byte plus SIGCONT (0x13) in the high byte — and golang.org/x/sys/unix's WaitStatus.Stopped() returns true for that encoding (it only checks high != SIGSTOP). With WCONTINUED enabled, every SIGCONT this loop issued to the child came back looking like a fresh Ctrl-Z, sending fence into an infinite [1]+ Stopped loop on every fg. WUNTRACED-only avoids the encoding mismatch and is all the main Ctrl-Z / fg flow needs; the spec's "third-party SIGCONT to fence" case still works because the resume handshake runs inline after each kill(self, SIGSTOP) returns.

Also adds:

• integration_jobcontrol_darwin_test.go: end-to-end PTY test that pins the Ctrl-Z / SIGCONT round-trip against ps state checks.
• wait4_probe_test.go: gated diagnostic that records the Darwin wait4 encoding so a future contributor who tries to re-add WCONTINUED sees why it breaks.

[jy-tan]

Thanks for the PR! A few comments, let me know what you think.

Since shouldManageHostTTYForeground is platform-agnostic, shall we also add an equivalent test for Linux?

[jy-tan]

do we want to set jobControlEnabled only if TIOCSPGRP to the child actually succeeds?

[voidexpr]

Done in 2623075 — moved the defer for restoring savedFgPgrp and the three jobControl* assignments inside the else branch, so a failed TIOCSPGRP falls back to execCmd.Wait() without activating job-control state that cannot function.

While testing this I found two more bugs in the same Ctrl-Z/bg flow (fixed in b5d31f7):

1. waitWithJobControl re-grant after bg: after resuming from self-SIGSTOP, the code always called TIOCSPGRP(childPgrp). With fg that is correct; with bg the shell is still the terminal foreground owner, so the call stole the TTY from the shell, causing its read() to return EIO (interpreted as EOF → shell exit). Fix: check TIOCGPGRP after resuming and only re-grant when fence is the foreground.

2. Deferred cleanup on exit: the defer in runCommand unconditionally called TIOCSPGRP(savedFgPgrp). In an interactive bash session savedFgPgrp is fence's own pgrp (bash assigns each pipeline its own pgrp before handing it the terminal), so the defer was stealing the TTY back from the shell right before exit, leaving an orphaned foreground pgrp and triggering EIO on the shell's next read. Fix: only call TIOCSPGRP(savedFgPgrp) when the child's pgrp is still the terminal foreground; skip it when the shell has already reclaimed the TTY.

[jy-tan]

thinking of a test that resumes and then observes the wrapped command completes (fence is not blocked forever waiting).

something of this shape:

func TestMacOS_CtrlZSuspendsFenceAndResumesChild(t *testing.T) {
	skipIfAlreadySandboxed(t)

	fenceBin := t.TempDir() + "/fence"
	build := exec.Command("go", "build", "-o", fenceBin, "../../cmd/fence") // #nosec G204
	build.Stdout = os.Stdout
	build.Stderr = os.Stderr
	if err := build.Run(); err != nil {
		t.Fatalf("failed to build fence: %v", err)
	}

	cmd := exec.Command(fenceBin, "sh", "-c", "printf 'READY\\n'; sleep 1; printf 'DONE\\n'") // #nosec G204

	ptmx, err := pty.Start(cmd)
	if err != nil {
		t.Fatalf("failed to start fence command with PTY: %v", err)
	}

	var output lockedBuffer
	done := make(chan struct{})
	go func() {
		_, _ = io.Copy(&output, ptmx)
		close(done)
	}()

	defer func() {
		_ = ptmx.Close()
		if cmd.Process != nil {
			// If the process is still stopped, SIGCONT first so Kill/Wait cleanup
			// cannot leave a stopped child behind.
			_ = syscall.Kill(cmd.Process.Pid, syscall.SIGCONT)
			_ = cmd.Process.Kill()
		}
		_ = cmd.Wait()
	}()

	if !waitForOutput(&output, "READY", 2*time.Second) {
		t.Fatalf("command did not become ready\noutput so far:\n%s", output.String())
	}

	if _, err := ptmx.Write([]byte{0x1A}); err != nil {
		t.Fatalf("failed to write Ctrl-Z: %v", err)
	}

	if !waitForProcessState(t, cmd.Process.Pid, "T", 2*time.Second) {
		t.Fatalf("fence did not enter stopped state after Ctrl-Z\noutput so far:\n%s", output.String())
	}

	if err := syscall.Kill(cmd.Process.Pid, syscall.SIGCONT); err != nil {
		t.Fatalf("failed to SIGCONT fence: %v", err)
	}

	if !waitForOutput(&output, "DONE", 5*time.Second) {
		t.Fatalf("child did not resume and complete\noutput so far:\n%s", output.String())
	}

	select {
	case <-done:
	case <-time.After(2 * time.Second):
		t.Fatalf("PTY output did not finish after child completed\noutput so far:\n%s", output.String())
	}

	if err := cmd.Wait(); err != nil {
		t.Fatalf("fence exited with error: %v\noutput:\n%s", err, output.String())
	}
}

with the following race-safe helpers:

type lockedBuffer struct {
	mu  sync.Mutex
	buf bytes.Buffer
}
func (b *lockedBuffer) Write(p []byte) (int, error) {
	b.mu.Lock()
	defer b.mu.Unlock()
	return b.buf.Write(p)
}
func (b *lockedBuffer) String() string {
	b.mu.Lock()
	defer b.mu.Unlock()
	return b.buf.String()
}
func waitForOutput(output *lockedBuffer, needle string, timeout time.Duration) bool {
	deadline := time.Now().Add(timeout)
	for time.Now().Before(deadline) {
		if strings.Contains(output.String(), needle) {
			return true
		}
		time.Sleep(25 * time.Millisecond)
	}
	return false
}

what do you think?

[voidexpr]

Added TestMacOS_CtrlZSuspendsFenceAndResumesChild in bf130b9, following exactly the shape you suggested. The race-safe helpers (lockedBuffer, waitForOutput) are extracted into a separate integration_jobcontrol_darwin_helpers_test.go (same package and build tag) so both Darwin test functions share one copy.

Also added integration_jobcontrol_linux_test.go (//go:build linux) with TestLinux_CtrlZSuspendsFence and TestLinux_CtrlZSuspendsFenceAndResumesChild — same PTY approach, same ps -o state= polling — to cover the stop/resume paths on Linux where WUNTRACED and TIOCSPGRP are also live.

[jy-tan]

Thanks for adding the stronger resume tests. One cleanup from my earlier suggestion: since CI runs with -race, async PTY capture should use the locked buffer anywhere the test goroutine reads it. The new full round-trip tests do this, but the older duplicate Test*_CtrlZSuspendsFence tests still use plain bytes.Buffer, can we either convert those too or remove the weaker duplicates?

[voidexpr]

Done in f68efcc — went with "remove the weaker duplicates": the Darwin/Linux harness is now a single //go:build darwin || linux helper (integration_jobcontrol_helpers_test.go) sharing one lockedBuffer, and the per-OS files are thin entry points, so there's no plain bytes.Buffer left and nothing's duplicated. (The locked-buffer conversion of the older tests itself landed earlier in 57dd5e7.)

[voidexpr]

So with claude code's help I tried to address the comments and found some extra issues. I've tested on both MacOS and Linux. Looking more I'm finding an issue I want to investigate more, I'll let you know when this is ready.

[voidexpr]

ok fixed one issue I found, it's in a reviewable state.

[jy-tan]

Thanks for this! Just a few more comments.

[jy-tan]

This SIGTTIN branch assumes the child is already the foreground pgrp. That’s true for the startup race, but not after Ctrl-Z -> bg; a background child reading from stdin should remain stopped, not be auto-SIGCONTed. Can we gate this on TIOCGPGRP == childPgrp before resuming?

i.e.,

if ws.StopSignal() == syscall.SIGTTIN {
	currentFg, fgErr := unix.IoctlGetInt(stdinFd, unix.TIOCGPGRP)
	if fgErr == nil && currentFg == childPgrp {
		if debug {
			fencelog.Printf("[fence:jobctl] child stopped on SIGTTIN after foreground handoff; resuming\n")
		}
		_ = syscall.Kill(-childPgrp, syscall.SIGCONT)
		continue
	}
	if debug {
		fencelog.Printf("[fence:jobctl] child stopped on SIGTTIN while not foreground (currentFg=%d); treating as job-control stop\n", currentFg)
	}
}

[voidexpr]

Done in a8cead2, implemented as you suggested: the SIGTTIN branch now reads TIOCGPGRP and only auto-SIGCONTs when currentFg == childPgrp (the foreground-handoff race). When the child stopped on SIGTTIN while not foreground (after Ctrl-Z → bg), it falls through to the normal job-control stop so it stays stopped and you can fg it.

One alternative Claude proposed: in the not-foreground case, continue and leave the child stopped without fence touching the terminal at all — instead of falling through to tcsetpgrp(self) + self-SIGSTOP while the shell currently owns the terminal. That sidesteps any momentary TTY handoff while the shell is foreground (the concern behind b5d31f7), but it doesn't surface the stop to the outer shell, so fg wouldn't recover the child. I went with your fall-through since the transient handoff is the same one the normal Ctrl-Z path already relies on — but if you'd rather not touch the terminal in the bg case, the continue variant is a one-line change. Preference?

[jy-tan]

Yes, leaning toward the continue variant because the shell already owns the terminal in this path, and a background fence calling TIOCSPGRP(self) seems like the thing we just fixed for Ctrl-Z -> bg. But I agree this needs proof either way. Can we add/manual-test Ctrl-Z -> bg, then a child stdin read, and verify the shell stays alive and fg recovers cleanly?

[voidexpr]

Went with the third path rather than a bare continue, because continue alone deadlocks on fg recovery: the child stays stopped on SIGTTIN, fence keeps running (never self-SIGSTOPs), so there is nothing for fg to bring back and nobody to re-grant the TTY.

[jy-tan]

If TIOCSPGRP(childPgrp) fails, we've already ignored SIGTTOU but never reset it. Can we reset SIGTTOU in the error branch before falling back to normal Wait()?

[voidexpr]

Done in f6f9f40. I register signal.Reset(SIGTTOU) with defer immediately after signal.Ignore(SIGTTOU), so it's reset on every exit path — including the TIOCSPGRP-failure fallback to Wait() you flagged. Used a defer rather than only the error branch so no later early-return can leak the ignored disposition; the foreground-pgrp restore stays success-only.

[jy-tan]

Thanks for adding the stronger resume tests. One cleanup from my earlier suggestion: since CI runs with -race, async PTY capture should use the locked buffer anywhere the test goroutine reads it. The new full round-trip tests do this, but the older duplicate Test*_CtrlZSuspendsFence tests still use plain bytes.Buffer, can we either convert those too or remove the weaker duplicates?

[voidexpr]

After addressing your three comments I ran a double Codex + Claude code review of this round and it turned up more than I expected. I folded the fixes into this PR, let me know if it's still manageable.

Extra changes:

• edc154f refactor: extract setForegroundIfOwner for guarded tcsetpgrp: companion to comment 1; dedups the "only tcsetpgrp if we still own the foreground" guard across main.go + jobcontrol.go (no behavior change).
• 6efc59c fix: stop signal forwarder once the job-control child is reaped: the job-control path reaps via wait4 and never calls execCmd.Wait(), leaving the SignalForwarder live during deferred cleanup; on macOS (no pidfd) it could signal a recycled PID.
• 91e8b2d fix: report 128+signal for a signaled child on the fallback path: the non-TTY path returned -1 for a signaled child while the job-control path returned 128+sig; aligned both (+ unit test).
• 6a9305b fix: log unexpected wait status in the job-control loop: debug-only default instead of silently re-blocking.

Two further findings were considered and rejected: assuming fg on a TIOCGPGRP error during resume (would risk re-introducing the b5d31f7 TTY-steal), and preserving exit status on a non-EINTR Wait4 error (can't occur for a child we own and reap once).

[voidexpr]

Let me know how it looks now. I did some manual tests (simple ones) and added some more automated tests

[jy-tan]

Thanks @voidexpr! I'll add to a new release within a couple days.