diff --git a/CHANGELOG.md b/CHANGELOG.md index f230964..4e63da8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -270,6 +270,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Commands no longer fail with "Error reclaiming terminal control: ... no such process" + when several mshell processes share one terminal, as under parallel build runners + (`redo`, `make -j`) or when a script is backgrounded. + mshell now transfers terminal control only when it is itself the terminal's current + foreground process group (the same gate bash and fish use), and a hand-back to a + previous foreground group that has since exited falls back to mshell's own group + instead of failing the command. + A reclaim problem is now at most a warning on stderr; the command's own exit code always stands. +- A fast pipeline whose processes finished before mshell could transfer terminal + control is no longer killed and reported as failed; the transfer is skipped, + since the work is already done. + Restoring terminal modes is now also protected from `SIGTTOU`, + which could previously stop the shell mid-cleanup when another process group owned the terminal. + A failure while closing the retained pipeline terminal handle is likewise now a warning, + never a failure of a pipeline whose commands succeeded. - Attempted to formally improve the semantics of job control and terminal control on both Linux and Windows. Should fix potential bugs when running TUI programs from within mshell scripts. - On Windows, a command name containing a forward slash (e.g. `./script.msh`) diff --git a/ai/terminal-control/ASSUMPTIONS.md b/ai/terminal-control/ASSUMPTIONS.md index 914c3fe..7098035 100644 --- a/ai/terminal-control/ASSUMPTIONS.md +++ b/ai/terminal-control/ASSUMPTIONS.md @@ -17,6 +17,12 @@ system versions change. - A shell foregrounds a job with `tcsetpgrp()`, observes stopped children with `waitpid(..., WUNTRACED)`, reclaims the terminal, and foregrounds a stopped job before sending `SIGCONT`. +- A process-group ID passed to `tcsetpgrp()` is a snapshot, not a stable handle: + the group can cease to exist at any time. POSIX documents `EPERM` for a group + ID with no member in the caller's session; Linux's `TIOCSPGRP` returns `ESRCH` + for a nonexistent group (observed in production, 2026-08-13, when restoring a + sibling shell's exited child group). Terminal hand-back must therefore treat + a dead target as an expected outcome, not an exceptional command failure. Sources: diff --git a/ai/terminal-control/ERRNO_AUDIT.md b/ai/terminal-control/ERRNO_AUDIT.md new file mode 100644 index 0000000..933d424 --- /dev/null +++ b/ai/terminal-control/ERRNO_AUDIT.md @@ -0,0 +1,143 @@ +# Errno audit of the terminal/job-control syscall surface + +Date: 2026-08-13. +Sources: Linux man-pages on the development machine (man-pages 6.x), POSIX +Issue 8, observed production behavior, and kernel behavior where the man pages +are incomplete. Method: enumerate every syscall the controller makes, list +every documented (and observed-undocumented) errno, and trace each to an +explicit policy in code and, where applicable, a transition in the TLA+ +models. A row with no policy is a finding. + +Verdicts: **OK** (explicit, correct policy), **FIXED** (gap found by this +audit, fixed 2026-08-13), **REC** (works, but a better policy is recommended +below), **N/A** (cannot occur at this call site, with reasoning). + +## POSIX backend + +### tcsetpgrp — `TIOCSPGRP` (`SetForegroundProcessGroup`, `RestoreForegroundProcessGroup`) + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| `ESRCH` (undocumented) | Target pgid fully reaped. Linux's `TIOCSPGRP` returns this; the tcsetpgrp man page does not list it. Observed in production 2026-08-13. | Acquire: skip the handoff (nothing left to foreground). Release: fall back to own group, warn only. | FIXED / OK | +| `EPERM` | Target pgid valid but not in caller's session. | Acquire: kill child, fail command. Release: fallback to own group (own group cannot be `EPERM`). | REC (R2) / OK | +| `ENOTTY` | fd no longer the caller's controlling terminal (hangup or dissociation after the resolve-time `CanControlTerminal` probe). | Acquire: kill child, fail command. Release: fallback also fails → warn, unblock input gate; later commands re-probe and skip. | REC (R2) / OK | +| `EBADF`, `EINVAL` | Bad fd / bad pgid value. Program error; fd is held via a duplicated handle in pipelines. | Same as `ENOTTY` paths. | OK | +| SIGTTOU (not an errno) | Caller is in a background group and does not ignore/block SIGTTOU: default action stops the shell. | Both call sites wrapped in `IgnoreSignalsForJobControl`. | OK | + +### tcgetpgrp — `TIOCGPGRP` (`CanControlTerminal`, `ShellOwnsTerminal`, first step of `SetForegroundProcessGroup`) + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| `ENOTTY`, `EBADF` | Not the controlling terminal / bad fd. | Resolve probe: endpoint is nil, no transaction. Ownership gate: reports non-owner, transaction skipped. Inside acquire: acquire fails (see tcsetpgrp rows). | OK | + +Read-only; does not raise SIGTTOU. + +### kill(-pgid, SIGCONT) — `ContinueProcessGroup` + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| `ESRCH` | Group fully reaped between `tcsetpgrp` and `SIGCONT`. A group whose members are zombies still exists (man kill: an existing process may be a zombie), so the single-command path — which reaps only after acquisition — cannot hit this; pipelines reap concurrently in per-stage goroutines and can. | Roll back the handoff, then skip: return no lease, no error. The job already finished. | FIXED | +| `EPERM` | Cannot signal any member. POSIX exempts `SIGCONT` within the sender's session, and the child group is always in our session. | Falls into the generic continue-failure path (rollback + fail). | N/A in practice, policy exists | +| `EINVAL` | Bad signal number. | Impossible (constant `SIGCONT`). | N/A | + +### kill(-pgid, SIGKILL) — `KillProcessGroup` + +Called only on already-failed launch/acquisition paths; the return value is +deliberately ignored at every call site (best-effort cleanup; `ESRCH` here +means the work is already done). **OK** — now documented as intentional. + +### tcgetattr — `TCGETS` (`CaptureTerminalMode`) + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| `ENOTTY`, `EBADF` | Terminal hung up between resolve and acquire. | Acquire fails before any state is touched; rollback is trivial. Evaluator kills the child and fails the command. | REC (R2) | + +Read-only; does not raise SIGTTOU. + +### tcsetattr — `TCSETSF` via `term.Restore` (`posixTerminalModeSnapshot.Restore`) + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| SIGTTOU (not an errno) | POSIX: `tcsetattr` from a background process group raises SIGTTOU; default action stops the process. Reachable whenever the restore runs while another group owns the terminal (e.g. Release handed the terminal back to a live foreign group recorded during the steal window). | Restore is now wrapped in `IgnoreSignalsForJobControl`, like the tcsetpgrp calls. | FIXED | +| `ENOTTY`, `EBADF`, `EIO`, `EINTR`, `EINVAL` | Terminal gone or hung up. | Release: warn; input gate stays blocked only when the terminal is otherwise alive (mode restored wrong is a real input hazard); on total terminal loss the gate is released. | OK | +| Partial success (returns 0) | man termios: `tcsetattr` "returns success if any of the requested changes could be carried out." | No policy possible without a verify-readback; accepted contract gap, recorded here. | OK (accepted) | + +### setpgid — child-side via `SysProcAttr` (leader `Pgid=0`, followers join leader) + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| `EPERM` (target group does not exist) | Follower joins after the leader's group died. | Prevented structurally: the leader waits for every stage to launch before it can be reaped (launch barrier). Residual failure surfaces as that stage's `cmd.Start` error → per-stage exit code. | OK | +| `EACCES`, `ESRCH`, `EINVAL` | Post-exec / wrong pid / negative pgid. | Not reachable from the child-side pre-exec call with Go's contract. | N/A | + +### waitpid — via `os/exec.Cmd.Wait` + +`EINTR` is retried inside the Go runtime; `ECHILD` cannot occur because Go +owns reaping for started commands. Non-`ExitError` failures map to +`ExitStartUnknown` with a printed diagnostic. **OK**. + +### dup / close — `DuplicateTerminalHandle`, `CloseTerminalHandle` (pipeline terminal retention) + +| Errno | Trigger | Policy | Verdict | +| --- | --- | --- | --- | +| dup: `EMFILE`, `ENOMEM`, `EBADF` | fd exhaustion / bad fd. | `registerTerminal` fails before the stage starts; command fails fast, nothing orphaned. | OK | +| close: `EBADF`, `EINTR`, `EIO` | On Linux the fd is closed even when `close` reports `EINTR`/`EIO`; the code correctly does not retry. | `closeTerminal` error is a stderr warning; the pipeline's own result stands. | FIXED (was R1) | + +### SIGTTOU/SIGTTIN protection — `IgnoreSignalsForJobControl` + +`signal.Ignore`/`signal.Reset` are process-wide and not reentrant: a nested +wrap would drop protection at the inner `Reset`. All current uses are +sequential and disjoint (verified). Recorded as a constraint for future code. + +## Windows backend + +The direct-console backend has a much smaller failure surface: the +"foreground" is an in-process marker, so `RestoreForegroundProcessGroup` is +infallible and `ESRCH`/SIGTTOU have no analogue. + +- `SetConsoleCtrlHandler` (inside `SetForegroundProcessGroup`): failure fails + acquisition; `KillProcessGroup` is a no-op, so only the immediate child is + killed — already documented in the code as awaiting Job Object support. +- `GetConsoleMode` capture skips per-handle failures and errors only when no + handle yields a mode; `SetConsoleMode` restore applies every saved mode and + reports the first failure. Reasonable; console modes are shared state that + other processes can also change (see the closed-world note in + MODEL_RESULTS.md). + +## Findings summary + +- **F1 (documented):** Linux `TIOCSPGRP` returns `ESRCH` for a reaped group; + the man page omits it. Recorded in ASSUMPTIONS.md. +- **F2 (fixed):** `term.Restore` (`tcsetattr`) ran without SIGTTOU protection; + a release that handed the terminal to a live foreign group could stop the + shell mid-`Release`. Now wrapped like the tcsetpgrp calls. +- **F3 (fixed):** a fast pipeline whose stages were reaped concurrently could + be fully gone before acquisition; the resulting `ESRCH` from + `tcsetpgrp`/`SIGCONT` killed and failed a job whose children exited 0. + `ESRCH` during acquisition now means "already finished, run without a + handoff" (with rollback where the handoff partially happened). + +Resolutions (maintainer decisions 2026-08-13): + +- **R1 (applied):** `closeTerminal` failure is now a warning; a successful + pipeline is never failed over it. +- **R2 (decided 2026-08-13: keep the kill):** non-`ESRCH` acquisition + failures (`ENOTTY`/`EIO` after a hangup in the resolve→acquire window, + `EPERM`) kill the child and fail the command. bash degrades to running the + job without foreground ownership — but bash can afford to: it has a job + table and `WUNTRACED` waits, so a child that later stops on `SIGTTIN` + becomes a recoverable stopped job. mshell's synchronous `cmd.Wait` would + hang forever on such a child, and a silent hang is worse than a wrong kill. + The kill also approximates `SIGHUP` semantics for children in their own + process groups when the terminal dies, and never leaves orphans. + Alternatives considered and declined for now: degrade only on + dead-terminal errnos (`ENOTTY`/`EBADF`/`EIO`, provably hang-free because + reads return `EIO` instead of raising `SIGTTIN`) — rejected as an extra + policy fork whose benefit (a rare race) does not outweigh orphaned + processes; full bash-style degrade — deferred. **Revisit when the + `jobs`/`fg`/`WUNTRACED` milestone lands, which makes full degrade safe.** +- **R3 (applied, modeled):** `POSIXTerminalControl.tla` now has a `reaped` + state distinct from zombie `exited`, early `ProcExits`, concurrent + `ReapProc`, the `~GroupDead` kernel contract on `GiveTerminal`, and the + `GiveTerminalTargetGone` skip transition; invariant + `UnsupervisedJobNeverOwnsTerminal` verified non-vacuous by mutation. See + MODEL_RESULTS.md. diff --git a/ai/terminal-control/IMPLEMENTATION_STATUS.md b/ai/terminal-control/IMPLEMENTATION_STATUS.md index 36a703f..fed3bb5 100644 --- a/ai/terminal-control/IMPLEMENTATION_STATUS.md +++ b/ai/terminal-control/IMPLEMENTATION_STATUS.md @@ -1,9 +1,57 @@ # Implementation status -Last updated: 2026-08-09. +Last updated: 2026-08-13. ## Implemented +### 2026-08-13 parallel-shell hardening + +Parallel `redo` builds exposed a cross-process race the 2026-08-09 milestone +missed: with several mshells sharing one PTY, each shell's recorded "previous +foreground process group" could be a sibling's transient child group, dead by +release time, so the restoring `tcsetpgrp` failed with ESRCH and the command +was failed even though its child exited 0. Changes: + +- Acquisition is gated on `tcgetpgrp(tty) == getpgrp()` (the bash/fish gate). + A shell that is not the terminal's current foreground owner skips the + foreground transaction entirely; its child runs as an ordinary background + process group. On Windows the gate is always open because the foreground + state is an in-process Ctrl-C marker, not shared kernel ownership. +- A failed hand-back to the recorded previous group falls back to restoring + the shell's own process group (the group bash and fish restore). Only if + both targets fail does `Release` report an error. +- Reclaim errors are warnings on stderr, never command failures; the child's + exit status always stands. This supersedes the earlier "restore the exact + previous foreground marker or fail" behavior. +- Policy change: after a double restore failure the shell input gate is now + released rather than held. Both targets failing means the terminal itself + is unusable (for example a closed PTY); holding the gate wedged every later + command behind a terminal that no longer exists. Mode-restore failure with + a live terminal still blocks shell input as before. + +### 2026-08-13 errno audit + +ERRNO_AUDIT.md traces every errno of every syscall the controller makes to an +explicit policy or a finding. Two further gaps found and fixed: + +- `term.Restore` (`tcsetattr`) ran without SIGTTOU protection and could stop + the shell when the terminal had been handed back to a live foreign group; + it is now wrapped in `IgnoreSignalsForJobControl` like the tcsetpgrp calls. +- A fast pipeline whose stages were reaped concurrently could have its whole + process group vanish before acquisition; the resulting ESRCH killed and + failed a job whose children exited 0. Acquisition-time ESRCH now means + "already finished": roll back any partial handoff and run without a lease. + +All three audit recommendations are resolved: R1 applied (closeTerminal +failure is a warning, never a pipeline failure), R3 modeled +(reaped-vs-zombie states in POSIXTerminalControl.tla), and R2 decided as +keep-the-kill — a non-ESRCH acquisition failure kills the child rather than +risking an unrecoverable SIGTTIN hang, since mshell has no stopped-job +recovery yet. R2 must be revisited when the `jobs`/`fg`/`WUNTRACED` +milestone lands; the full trade-off is in ERRNO_AUDIT.md. + +### 2026-08-09 milestone + - Resolved stdin/stdout/stderr metadata remains attached to the `exec.Cmd` streams after redirects and merges are applied. - Terminal selection distinguishes an arbitrary TTY from the session's @@ -38,6 +86,20 @@ Last updated: 2026-08-09. isolation mechanism for interactive background jobs, not the foreground handoff mechanism. +Verification added with the hardening: + +- A PTY integration test starts six shells sharing one PTY, each running a + staggered short external command, and requires all six to succeed. Before + the fix it reproduced the production failure exactly (three to four of six + failing with "no such process" on reclaim). +- Controller unit tests cover the skipped acquisition for a non-owner shell, + the fallback restore order (dead previous group, then own group), and the + combined error when both hand-back targets fail. +- `POSIXTerminalControl.tla` now models the foreign owner, the steal window, + the dying previous-owner group, the ownership gate, and the fallback; all + six bounded TLC configurations pass, including the new shared-terminal + profile. See MODEL_RESULTS.md. + ## Verification currently passing - Controller unit tests inject mode capture, acquisition, continue, rollback, diff --git a/ai/terminal-control/MODEL_RESULTS.md b/ai/terminal-control/MODEL_RESULTS.md index 45adf15..559992b 100644 --- a/ai/terminal-control/MODEL_RESULTS.md +++ b/ai/terminal-control/MODEL_RESULTS.md @@ -2,13 +2,14 @@ Tool: TLA+ tools 1.7.4, TLC 2.19 (`5a47802`), Java 17. -Last complete run: 2026-08-09. +Last complete run: 2026-08-13. | Model | Configured size | Distinct states | Result | | --- | ---: | ---: | --- | | `TerminalControl` | 2 jobs | 204 | all configured invariants hold | -| `POSIXTerminalControl` | 2 processes; shell stdin non-TTY, child stdin TTY | 58 | all configured invariants hold | -| `POSIXTerminalControl` | 2 processes; child stdin non-TTY | 2 | all configured invariants hold | +| `POSIXTerminalControl` | 2 processes; shell non-owner start allowed as below; shell stdin non-TTY, child stdin TTY | 720 | all configured invariants hold | +| `POSIXTerminalControl` | 2 processes; child stdin non-TTY | 4 | all configured invariants hold | +| `POSIXTerminalControl` (`POSIXSharedTerminal.cfg`) | 2 processes; shell does not own the terminal; child stdin TTY | 152 | all configured invariants hold | | `WindowsTerminalControl` | one foreground job | 11 | all configured invariants hold | | `StreamLifecycle` | 2 processes, 5 stream handles, retained terminal | 36 | all configured invariants hold | @@ -33,17 +34,72 @@ The model now has a single `controlJob` reservation spanning resolution through recovery. This is a design requirement for the future Go controller, not merely a modeling convenience. +## Counterexample found in production (2026-08-13) + +Parallel `redo` builds (several mshells sharing one PTY) hit a race the models +could not represent, because the modeled universe was closed: + +- `Init` fixed `ttyForeground = ShellPgrp`: the shell was assumed to start as + the terminal's foreground owner. +- `TypeOK` restricted `ttyForeground` to `{ShellPgrp, JobPgrp}`: no foreign + process group existed, so no sibling could own or steal the terminal and no + recorded previous owner could be anything but the shell itself. +- `ReclaimTerminal` could not fail: restoring `ShellPgrp` always succeeds, so + the ESRCH hand-back failure had no modeled transition, and the implementation + question "what severity is a failed restore?" was never posed. + +Inside that universe, "restore the exact previous foreground process group" and +"restore your own process group" are indistinguishable — the previous owner is +always the shell. The implementation chose the former; outside the model the +previous owner can be a sibling's transient child group that is dead by release +time. REQUIREMENTS.md had listed "shells started outside the foreground +process group" as a required profile, and the boundaries section below listed +nested shells as missing state, but no configuration exercised either. + +`POSIXTerminalControl.tla` now models the environment: a foreign process group +(`OtherPgrp`) that may own the terminal from the start +(`ShellStartsForeground = FALSE`), may steal it in the window between the +ownership check and `tcsetpgrp` and while the job is foreground, and may exit +at any time, making the recorded previous owner a dead pgid. The two +production fixes are modeled as transitions: `CheckOwnershipFails` (a +non-owner shell skips the handoff) and +`ReclaimHandBackFails`/`ReclaimFallbackToOwnGroup` (an ESRCH hand-back falls +back to the shell's own group). The new invariant +`NonOwnerShellNeverForegrounds` fails within seconds if the ownership gate is +removed from the model (verified by mutation). + +## Counterexample found by the errno audit (2026-08-13, F3) + +The audit's second revision adds early reaping: processes may exit from the +moment they launch, and `ReapProc` may turn a zombie into a fully reaped +process at any time (pipelines reap concurrently). A zombie keeps its pgid — +`tcsetpgrp` to a zombie-only group succeeds — while a fully reaped group makes +it fail with Linux's undocumented ESRCH. `GiveTerminal` now carries the +kernel contract as a `~GroupDead` guard, and `GiveTerminalTargetGone` models +the production fix: an ESRCH handoff means the job already finished, so the +shell runs it unsupervised instead of killing and failing it. The invariant +`UnsupervisedJobNeverOwnsTerminal` covers both skip paths (non-owner shell, +reaped group); mutating `GiveTerminalTargetGone` to point the terminal at the +job violates it immediately, confirming the reaped-at-handoff states are +reachable and the invariant is not vacuous. + ## Boundaries and unfinished proof work - These are exhaustive finite-state safety checks, not unbounded TLAPS proofs. - The current platform modules are contract models, not yet mechanically checked refinement mappings to `TerminalControl`. +- The external-steal window is deliberately limited to the `ownedReady` and + `foreground` phases. A fully adversarial environment that can take the + terminal at any time makes every reader-ownership invariant unsatisfiable; + the kernel answers that case with `SIGTTIN`/`SIGTTOU`, and the shell's own + stopped-by-signal states are not modeled. Reclaiming to a live foreign + owner leaves the model in a `resuming` dead end for the same reason. - Progress requirements are documented but not checked yet. Fairness must be stated carefully because children are allowed to run forever or retain handles. - The Windows model covers reader quiescence in a shared console. A separate, larger ConPTY relay model is required before claiming full Windows isolation. -- Terminal resize, nested shells, signal masks, per-process stopped/continued - aggregation, mode-snapshot failure, and shutdown policy need additional state. +- Terminal resize, signal masks, per-process stopped/continued aggregation, + mode-snapshot failure, and shutdown policy need additional state. - The Go controller and failure-injection tests mirror the principal ownership transitions, but no refinement checker has mechanically proved that every Go execution implements the TLA+ specification. Generated trace replay remains diff --git a/ai/terminal-control/POSIXNonTerminal.cfg b/ai/terminal-control/POSIXNonTerminal.cfg index 0cab321..f785ab2 100644 --- a/ai/terminal-control/POSIXNonTerminal.cfg +++ b/ai/terminal-control/POSIXNonTerminal.cfg @@ -2,6 +2,8 @@ CONSTANTS Procs = {p1, p2} ShellPgrp = shellPgrp JobPgrp = jobPgrp + OtherPgrp = otherPgrp + ShellStartsForeground = TRUE ShellStdinIsTTY = TRUE ChildStdinIsTTY = FALSE @@ -15,3 +17,5 @@ INVARIANTS RunningProcessesAreGroupedBeforeForeground PromptModeIsRestored ResolvedChildEndpointControlsHandoff + NonOwnerShellNeverForegrounds + UnsupervisedJobNeverOwnsTerminal diff --git a/ai/terminal-control/POSIXSharedTerminal.cfg b/ai/terminal-control/POSIXSharedTerminal.cfg new file mode 100644 index 0000000..66482f7 --- /dev/null +++ b/ai/terminal-control/POSIXSharedTerminal.cfg @@ -0,0 +1,25 @@ +\* The parallel-runner profile discovered 2026-08-13: the shell shares a +\* terminal it does not own (redo -j, make -j, a backgrounded script). The +\* ownership gate must keep it from ever foregrounding a job or recording a +\* previous owner to restore. +CONSTANTS + Procs = {p1, p2} + ShellPgrp = shellPgrp + JobPgrp = jobPgrp + OtherPgrp = otherPgrp + ShellStartsForeground = FALSE + ShellStdinIsTTY = TRUE + ChildStdinIsTTY = TRUE + +INIT Init +NEXT Next + +INVARIANTS + TypeOK + ShellReaderHasForeground + JobForegroundPausesShell + RunningProcessesAreGroupedBeforeForeground + PromptModeIsRestored + ResolvedChildEndpointControlsHandoff + NonOwnerShellNeverForegrounds + UnsupervisedJobNeverOwnsTerminal diff --git a/ai/terminal-control/POSIXTerminalControl.cfg b/ai/terminal-control/POSIXTerminalControl.cfg index 7bdbd0c..4229cc2 100644 --- a/ai/terminal-control/POSIXTerminalControl.cfg +++ b/ai/terminal-control/POSIXTerminalControl.cfg @@ -2,6 +2,8 @@ CONSTANTS Procs = {p1, p2} ShellPgrp = shellPgrp JobPgrp = jobPgrp + OtherPgrp = otherPgrp + ShellStartsForeground = TRUE ShellStdinIsTTY = FALSE ChildStdinIsTTY = TRUE @@ -15,3 +17,5 @@ INVARIANTS RunningProcessesAreGroupedBeforeForeground PromptModeIsRestored ResolvedChildEndpointControlsHandoff + NonOwnerShellNeverForegrounds + UnsupervisedJobNeverOwnsTerminal diff --git a/ai/terminal-control/POSIXTerminalControl.tla b/ai/terminal-control/POSIXTerminalControl.tla index bd761ab..fc489a5 100644 --- a/ai/terminal-control/POSIXTerminalControl.tla +++ b/ai/terminal-control/POSIXTerminalControl.tla @@ -1,43 +1,91 @@ ----------------------- MODULE POSIXTerminalControl ----------------------- EXTENDS FiniteSets, TLC -CONSTANTS Procs, ShellPgrp, JobPgrp, ShellStdinIsTTY, ChildStdinIsTTY +(***************************************************************************) +(* The 2026-08-13 parallel-runner incident (redo -j spawning many mshells *) +(* that share one PTY) showed the original model's universe was closed: it *) +(* assumed the shell starts as the terminal's foreground owner, that only *) +(* the shell and its job ever own the terminal, and that the reclaiming *) +(* tcsetpgrp cannot fail. This revision adds the environment: *) +(* *) +(* - OtherPgrp: a foreign process group in the same session (a parallel *) +(* runner, a sibling shell, or a sibling's transient child group); *) +(* - ShellStartsForeground = FALSE: the shell may not own the terminal; *) +(* - a steal window between the ownership check and tcsetpgrp, and *) +(* while the job runs (siblings ignore SIGTTOU, so their tcsetpgrp *) +(* always succeeds); *) +(* - OtherGroupExits: the recorded previous owner is a snapshot of a *) +(* pgid, not a stable handle, and can be dead at hand-back time; and *) +(* - the two production fixes: a shell that does not own the terminal *) +(* skips the handoff entirely, and a hand-back that fails because the *) +(* recorded group died falls back to the shell's own group. *) +(* *) +(* Deliberate boundary: the steal window is limited to the phases where *) +(* the discovered race lives ("ownedReady" and "foreground"). A fully *) +(* adversarial environment that can steal at any time makes every reader- *) +(* ownership invariant unsatisfiable; in reality the kernel answers that *) +(* case with SIGTTIN/SIGTTOU, which this model does not simulate for the *) +(* shell's own reads. *) +(* *) +(* Revision 2 (errno audit finding F3): processes may exit as soon as they *) +(* are launched, and pipelines reap concurrently, so the job's own group *) +(* can be fully reaped before the handoff. "exited" is a zombie — it *) +(* still holds its pgid, so tcsetpgrp to the group succeeds — while *) +(* "reaped" does not, and tcsetpgrp to a fully reaped group fails with *) +(* the (undocumented on Linux) ESRCH. The production fix models as *) +(* GiveTerminalTargetGone: an ESRCH handoff means the job already *) +(* finished, so the shell runs it unsupervised instead of failing it. *) +(***************************************************************************) + +CONSTANTS Procs, ShellPgrp, JobPgrp, OtherPgrp, + ShellStartsForeground, ShellStdinIsTTY, ChildStdinIsTTY ASSUME /\ Procs # {} /\ ShellPgrp # JobPgrp + /\ OtherPgrp \notin {ShellPgrp, JobPgrp} + /\ ShellStartsForeground \in BOOLEAN /\ ShellStdinIsTTY \in BOOLEAN /\ ChildStdinIsTTY \in BOOLEAN VARIABLES phase, shellReader, ttyForeground, procState, grouped, - terminalMode, failure + terminalMode, failure, otherAlive, savedPrev vars == <> + terminalMode, failure, otherAlive, savedPrev>> + +ProcStates == {"idle", "running", "stopped", "exited", "reaped", "startFailed"} + +NoSave == "none" -ProcStates == {"idle", "running", "stopped", "exited", "startFailed"} +\* "exited" is a zombie: it still occupies its pgid, so the group remains a +\* valid tcsetpgrp/kill target. Only when every started member is reaped does +\* the group cease to exist (kernel returns ESRCH). +GroupDead == \A p \in Procs: procState[p] \in {"reaped", "startFailed"} Init == /\ phase = "idle" /\ shellReader = "active" - /\ ttyForeground = ShellPgrp + /\ ttyForeground = IF ShellStartsForeground THEN ShellPgrp ELSE OtherPgrp /\ procState = [p \in Procs |-> "idle"] /\ grouped = [p \in Procs |-> FALSE] /\ terminalMode = "shellRaw" /\ failure = "none" + /\ otherAlive = TRUE + /\ savedPrev = NoSave ResolveTerminalEndpoint == /\ phase = "idle" /\ ChildStdinIsTTY /\ phase' = "resolved" /\ UNCHANGED <> + terminalMode, failure, otherAlive, savedPrev>> ResolveNonTerminalEndpoint == /\ phase = "idle" /\ ~ChildStdinIsTTY /\ phase' = "done" /\ UNCHANGED <> + terminalMode, failure, otherAlive, savedPrev>> PauseShell == /\ phase = "resolved" @@ -45,7 +93,8 @@ PauseShell == /\ shellReader' = "paused" /\ terminalMode' = "shellCooked" /\ phase' = "launching" - /\ UNCHANGED <> + /\ UNCHANGED <> StartProc(p) == /\ phase = "launching" @@ -53,52 +102,126 @@ StartProc(p) == \* Go's child-side Setpgid contract makes membership effective before exec. /\ procState' = [procState EXCEPT ![p] = "running"] /\ grouped' = [grouped EXCEPT ![p] = TRUE] - /\ UNCHANGED <> + /\ UNCHANGED <> StartProcFails(p) == /\ phase = "launching" /\ procState[p] = "idle" /\ procState' = [procState EXCEPT ![p] = "startFailed"] /\ grouped' = [grouped EXCEPT ![p] = FALSE] - /\ UNCHANGED <> + /\ UNCHANGED <> LaunchComplete == /\ phase = "launching" /\ \A p \in Procs: procState[p] # "idle" - /\ \E p \in Procs: procState[p] = "running" + \* A member that already exited (or was even reaped) still counts as + \* launched; the barrier counts launches, not survivors. + /\ \E p \in Procs: procState[p] \in {"running", "exited", "reaped"} /\ phase' = "groupReady" /\ UNCHANGED <> + terminalMode, failure, otherAlive, savedPrev>> AllStartFailed == /\ phase = "launching" /\ \A p \in Procs: procState[p] = "startFailed" /\ phase' = "reclaiming" /\ failure' = "start" - /\ UNCHANGED <> + /\ UNCHANGED <> -GiveTerminal == +\* The production gate: the foreground transaction runs only when the shell's +\* own process group currently owns the terminal (tcgetpgrp == getpgrp). +CheckOwnershipPasses == /\ phase = "groupReady" - /\ shellReader = "paused" /\ ttyForeground = ShellPgrp + /\ phase' = "ownedReady" + /\ UNCHANGED <> + +\* A non-owner shell skips the handoff entirely: no tcsetpgrp, no mode save, +\* no reclamation. Its child runs as an ordinary background process group. +CheckOwnershipFails == + /\ phase = "groupReady" + /\ ttyForeground # ShellPgrp + /\ phase' = "unsupervised" + /\ UNCHANGED <> + +\* A sibling in the session hands the terminal to its own child. Siblings +\* ignore SIGTTOU while doing so, so this succeeds regardless of the current +\* owner. This is the TOCTOU window after our ownership check, and it can +\* also happen while our job is foreground. +ExternalTakesTerminal == + /\ phase \in {"ownedReady", "foreground"} + /\ otherAlive + /\ ttyForeground # OtherPgrp + /\ ttyForeground' = OtherPgrp + /\ UNCHANGED <> + +\* A pgid recorded at acquisition is a snapshot, not a stable handle: the +\* foreign group can exit at any time, making a later hand-back fail (ESRCH). +OtherGroupExits == + /\ otherAlive + /\ otherAlive' = FALSE + /\ UNCHANGED <> + +\* tcsetpgrp succeeds even if a sibling stole the terminal inside the window; +\* the snapshot of the previous owner is whatever tcgetpgrp returned then. +\* The ~GroupDead guard is the kernel contract, not a shell decision: a group +\* kept alive by zombies is a valid target, a fully reaped one is ESRCH. +GiveTerminal == + /\ phase = "ownedReady" + /\ shellReader = "paused" + /\ ~GroupDead /\ \A p \in Procs: procState[p] = "running" => grouped[p] + /\ savedPrev' = ttyForeground /\ ttyForeground' = JobPgrp /\ terminalMode' = "jobMode" /\ phase' = "foreground" - /\ UNCHANGED <> + /\ UNCHANGED <> + +\* Errno-audit fix F3: the handoff tcsetpgrp fails with ESRCH because the +\* job's own group was fully reaped (fast pipeline, concurrent reaping). The +\* job already finished; the shell runs it unsupervised instead of killing a +\* job whose children exited 0. The terminal was not touched. +GiveTerminalTargetGone == + /\ phase = "ownedReady" + /\ shellReader = "paused" + /\ GroupDead + /\ phase' = "unsupervised" + /\ UNCHANGED <> TcsetpgrpFails == - /\ phase = "groupReady" + /\ phase = "ownedReady" /\ phase' = "reclaiming" /\ failure' = "tcsetpgrp" - /\ UNCHANGED <> + /\ UNCHANGED <> +\* A process may exit the instant it is launched, well before the shell +\* checks ownership or hands the terminal over (errno-audit finding F3). ProcExits(p) == - /\ phase = "foreground" + /\ phase \in {"launching", "groupReady", "ownedReady", + "foreground", "unsupervised"} /\ procState[p] = "running" /\ procState' = [procState EXCEPT ![p] = "exited"] /\ UNCHANGED <> + terminalMode, failure, otherAlive, savedPrev>> + +\* Pipelines reap concurrently (per-stage goroutines), so a zombie can turn +\* into a fully reaped process at any point, including mid-transaction. The +\* single-command path reaps only after release; the model checks the worst +\* case. +ReapProc(p) == + /\ procState[p] = "exited" + /\ procState' = [procState EXCEPT ![p] = "reaped"] + /\ UNCHANGED <> JobStops == /\ phase = "foreground" @@ -106,52 +229,116 @@ JobStops == /\ procState' = [p \in Procs |-> IF procState[p] = "running" THEN "stopped" ELSE procState[p]] /\ phase' = "reclaiming" - /\ UNCHANGED <> + /\ UNCHANGED <> JobExited == /\ phase = "foreground" - /\ \A p \in Procs: procState[p] \in {"exited", "startFailed"} + /\ \A p \in Procs: procState[p] \in {"exited", "reaped", "startFailed"} /\ phase' = "reclaiming" /\ UNCHANGED <> + terminalMode, failure, otherAlive, savedPrev>> + +\* An unsupervised (skipped-handoff) job completes without the shell touching +\* terminal ownership or modes. A child that read the terminal would be +\* stopped by SIGTTIN, which this model leaves out of scope. +UnsupervisedComplete == + /\ phase = "unsupervised" + /\ \A p \in Procs: procState[p] \in {"exited", "reaped", "startFailed"} + /\ phase' = "resuming" + /\ UNCHANGED <> + +\* No handoff happened (start failure or failed tcsetpgrp), so there is no +\* previous owner to restore. +ReclaimNoHandoff == + /\ phase = "reclaiming" + /\ shellReader = "paused" + /\ savedPrev = NoSave + /\ terminalMode' = "shellCooked" + /\ phase' = "resuming" + /\ UNCHANGED <> -ReclaimTerminal == +\* Hand the terminal back to the recorded previous owner while it is alive. +ReclaimRestoresPrev == /\ phase = "reclaiming" /\ shellReader = "paused" - /\ ttyForeground \in {ShellPgrp, JobPgrp} + /\ savedPrev # NoSave + /\ (savedPrev = ShellPgrp \/ (savedPrev = OtherPgrp /\ otherAlive)) + /\ ttyForeground' = savedPrev + /\ terminalMode' = "shellCooked" + /\ phase' = "resuming" + /\ UNCHANGED <> + +\* The recorded previous owner died: the hand-back tcsetpgrp fails with ESRCH. +\* This is bookkeeping, not a command failure. +ReclaimHandBackFails == + /\ phase = "reclaiming" + /\ shellReader = "paused" + /\ savedPrev = OtherPgrp + /\ ~otherAlive + /\ phase' = "reclaimFallback" + /\ UNCHANGED <> + +\* The production fallback: restore the shell's own group, which always +\* exists. This is also the group bash and fish restore unconditionally. +ReclaimFallbackToOwnGroup == + /\ phase = "reclaimFallback" /\ ttyForeground' = ShellPgrp /\ terminalMode' = "shellCooked" /\ phase' = "resuming" - /\ UNCHANGED <> + /\ UNCHANGED <> +\* A foreground-owner shell resumes its reader only once it owns the terminal +\* again. If reclamation legitimately restored a live foreign owner, the +\* reader stays paused; resuming to read a terminal owned by someone else is +\* the SIGTTIN case outside this model. A shell that never owned the terminal +\* resumes reading its (possibly non-terminal) stdin without touching modes. ResumeShell == /\ phase = "resuming" - /\ ttyForeground = ShellPgrp + /\ ShellStartsForeground => ttyForeground = ShellPgrp /\ shellReader' = "active" - /\ terminalMode' = "shellRaw" + /\ terminalMode' = IF ShellStartsForeground THEN "shellRaw" ELSE terminalMode /\ phase' = "done" - /\ UNCHANGED <> + /\ UNCHANGED <> Next == ResolveTerminalEndpoint \/ ResolveNonTerminalEndpoint \/ PauseShell \/ - (\E p \in Procs: StartProc(p) \/ StartProcFails(p) \/ ProcExits(p)) \/ - LaunchComplete \/ AllStartFailed \/ GiveTerminal \/ TcsetpgrpFails \/ - JobStops \/ JobExited \/ ReclaimTerminal \/ ResumeShell + (\E p \in Procs: StartProc(p) \/ StartProcFails(p) \/ ProcExits(p) + \/ ReapProc(p)) \/ + LaunchComplete \/ AllStartFailed \/ + CheckOwnershipPasses \/ CheckOwnershipFails \/ + ExternalTakesTerminal \/ OtherGroupExits \/ + GiveTerminal \/ GiveTerminalTargetGone \/ TcsetpgrpFails \/ + JobStops \/ JobExited \/ UnsupervisedComplete \/ + ReclaimNoHandoff \/ ReclaimRestoresPrev \/ ReclaimHandBackFails \/ + ReclaimFallbackToOwnGroup \/ ResumeShell Spec == Init /\ [][Next]_vars TypeOK == - /\ phase \in {"idle", "resolved", "launching", "groupReady", - "foreground", "reclaiming", "resuming", "done"} + /\ phase \in {"idle", "resolved", "launching", "groupReady", "ownedReady", + "foreground", "unsupervised", "reclaiming", + "reclaimFallback", "resuming", "done"} /\ shellReader \in {"active", "paused"} - /\ ttyForeground \in {ShellPgrp, JobPgrp} + /\ ttyForeground \in {ShellPgrp, JobPgrp, OtherPgrp} /\ procState \in [Procs -> ProcStates] /\ grouped \in [Procs -> BOOLEAN] /\ terminalMode \in {"shellRaw", "shellCooked", "jobMode"} /\ failure \in {"none", "start", "tcsetpgrp"} + /\ otherAlive \in BOOLEAN + /\ savedPrev \in {NoSave, ShellPgrp, OtherPgrp} +\* Meaningful only for a shell that owns its terminal; a shell sharing a +\* terminal it does not own cannot enforce anything about foreign ownership. ShellReaderHasForeground == - shellReader = "active" => ttyForeground = ShellPgrp + ShellStartsForeground => + (shellReader = "active" => ttyForeground = ShellPgrp) JobForegroundPausesShell == ttyForeground = JobPgrp => shellReader = "paused" @@ -161,10 +348,29 @@ RunningProcessesAreGroupedBeforeForeground == \A p \in Procs: procState[p] = "running" => grouped[p] PromptModeIsRestored == - shellReader = "active" => terminalMode = "shellRaw" + ShellStartsForeground => + (shellReader = "active" => terminalMode = "shellRaw") ResolvedChildEndpointControlsHandoff == ~ChildStdinIsTTY => phase \notin {"resolved", "launching", "groupReady", - "foreground", "reclaiming", "resuming"} + "ownedReady", "foreground", + "unsupervised", "reclaiming", + "reclaimFallback", "resuming"} + +\* The gate's guarantee: a shell that never owned the terminal never +\* foregrounds a job, never applies job terminal modes, and never records a +\* previous owner it would later have to restore. +NonOwnerShellNeverForegrounds == + ~ShellStartsForeground => + /\ ttyForeground # JobPgrp + /\ terminalMode # "jobMode" + /\ savedPrev = NoSave + +\* Both skip paths (non-owner shell, ESRCH on a reaped group) run the job +\* without ever pointing the terminal at it or applying job terminal modes. +UnsupervisedJobNeverOwnsTerminal == + phase = "unsupervised" => + /\ ttyForeground # JobPgrp + /\ terminalMode # "jobMode" ============================================================================= diff --git a/ai/terminal-control/README.md b/ai/terminal-control/README.md index c4fdc1d..bb23ef9 100644 --- a/ai/terminal-control/README.md +++ b/ai/terminal-control/README.md @@ -44,6 +44,8 @@ different location. Production conformance progress is tracked in [IMPLEMENTATION_STATUS.md](IMPLEMENTATION_STATUS.md). +Every errno of every controller syscall is traced to a policy in +[ERRNO_AUDIT.md](ERRNO_AUDIT.md); re-run that audit when a syscall is added. The chosen Windows foreground architecture and the rejected nested-ConPTY alternative are recorded in [WINDOWS_DIRECT_HANDOFF.md](WINDOWS_DIRECT_HANDOFF.md). diff --git a/ai/terminal-control/REQUIREMENTS.md b/ai/terminal-control/REQUIREMENTS.md index f3bdaab..a7dd048 100644 --- a/ai/terminal-control/REQUIREMENTS.md +++ b/ai/terminal-control/REQUIREMENTS.md @@ -41,6 +41,17 @@ the target of a control event. partial pipeline completion does not destroy the job prematurely. 12. Shutdown and cancellation terminate or detach jobs according to an explicit policy and do not leak goroutines, handles, processes, or terminal modes. +13. The shell transfers terminal ownership only while its own process group is + the terminal's current foreground process group (`tcgetpgrp == getpgrp`), + the same gate bash and fish apply. A shell that shares a terminal it does + not own — one of several parallel shells under `redo`/`make -j`, or a + backgrounded script — runs its children without a foreground transaction. +14. The recorded previous foreground process group is a snapshot of a pgid, not + a stable handle: it may belong to a sibling's transient child and be dead by + hand-back time. A failed hand-back (ESRCH) is bookkeeping, not a command + failure; the shell falls back to restoring its own process group, and a + child's successful exit status is never overridden by reclaim errors. + (Both added 2026-08-13 after parallel `redo` builds hit the reclaim race.) ## Progress requirements diff --git a/ai/terminal-control/check.sh b/ai/terminal-control/check.sh index 9747d1e..b1279b4 100755 --- a/ai/terminal-control/check.sh +++ b/ai/terminal-control/check.sh @@ -15,3 +15,6 @@ done java -XX:+UseParallelGC -cp "$jar" tlc2.TLC -workers 1 -deadlock \ -config POSIXNonTerminal.cfg POSIXTerminalControl.tla + +java -XX:+UseParallelGC -cp "$jar" tlc2.TLC -workers 1 -deadlock \ + -config POSIXSharedTerminal.cfg POSIXTerminalControl.tla diff --git a/mshell/Evaluator.go b/mshell/Evaluator.go index c05a8a5..2eaedb2 100644 --- a/mshell/Evaluator.go +++ b/mshell/Evaluator.go @@ -4163,10 +4163,11 @@ func RunProcess(list MShellList, context ExecuteContext, state *EvalState) (Eval waitErr := cmd.Wait() - // Reclaim the exact terminal and previous process group recorded by the - // acquisition transaction before evaluation can resume shell input. + // Reclaim the terminal before evaluation can resume shell input. A + // reclaim failure is shell bookkeeping and must not override the + // child's own result: the child may have exited 0. if err := foregroundLease.Release(); err != nil { - return state.FailWithMessage(fmt.Sprintf("Error reclaiming terminal control: %s\n", err)), 1, commandSubWriter.Bytes(), stderrBuffer.Bytes() + fmt.Fprintf(os.Stderr, "Warning: reclaiming terminal control: %s\n", err) } if waitErr != nil { @@ -4521,11 +4522,13 @@ func (state *EvalState) RunPipeline(MShellPipe MShellPipe, context ExecuteContex if foregroundErr != nil { return state.FailWithMessage(fmt.Sprintf("Error acquiring pipeline terminal control: %s\n", foregroundErr)), 1, stdoutBytes, stderrBytes } + // Reclaim and handle-close failures are shell bookkeeping and must not + // override the pipeline's own result. if reclaimErr != nil { - return state.FailWithMessage(fmt.Sprintf("Error reclaiming pipeline terminal control: %s\n", reclaimErr)), 1, stdoutBytes, stderrBytes + fmt.Fprintf(os.Stderr, "Warning: reclaiming pipeline terminal control: %s\n", reclaimErr) } if closeTerminalErr != nil { - return state.FailWithMessage(fmt.Sprintf("Error closing retained pipeline terminal: %s\n", closeTerminalErr)), 1, stdoutBytes, stderrBytes + fmt.Fprintf(os.Stderr, "Warning: closing retained pipeline terminal: %s\n", closeTerminalErr) } // Check for errors diff --git a/mshell/Pathbin_darwin.go b/mshell/Pathbin_darwin.go index e7cacf8..92176ea 100644 --- a/mshell/Pathbin_darwin.go +++ b/mshell/Pathbin_darwin.go @@ -292,6 +292,23 @@ func CanControlTerminal(fd int) bool { return err == nil } +// ShellOwnsTerminal reports whether this process's group is the terminal's +// current foreground process group. This is the standard bash/fish gate: a +// shell that is not the foreground owner (a script under redo/make -j, a +// backgrounded script) must not hand the terminal to its children, because +// grabbing a terminal owned by someone else is exactly how parallel shells +// clobber each other's foreground state. +func ShellOwnsTerminal(ttyFd int) bool { + pgid, err := unix.IoctlGetInt(ttyFd, unix.TIOCGPGRP) + return err == nil && pgid == syscall.Getpgrp() +} + +// ShellProcessGroup returns this shell's own process group, the fallback +// hand-back target when the recorded previous foreground group has exited. +func ShellProcessGroup() int { + return syscall.Getpgrp() +} + func DuplicateTerminalHandle(fd int) (int, error) { return unix.Dup(fd) } @@ -314,6 +331,11 @@ func CaptureTerminalMode(fd int) (TerminalModeSnapshot, error) { } func (snapshot *posixTerminalModeSnapshot) Restore() error { + // tcsetattr from a background process group raises SIGTTOU (default action: + // stop). At restore time the terminal may already belong to another group, + // so the same protection used around tcsetpgrp applies here. + restoreSignals := IgnoreSignalsForJobControl() + defer restoreSignals() return term.Restore(snapshot.fd, snapshot.state) } diff --git a/mshell/Pathbin_linux.go b/mshell/Pathbin_linux.go index 88bc763..e962816 100644 --- a/mshell/Pathbin_linux.go +++ b/mshell/Pathbin_linux.go @@ -283,6 +283,23 @@ func CanControlTerminal(fd int) bool { return err == nil } +// ShellOwnsTerminal reports whether this process's group is the terminal's +// current foreground process group. This is the standard bash/fish gate: a +// shell that is not the foreground owner (a script under redo/make -j, a +// backgrounded script) must not hand the terminal to its children, because +// grabbing a terminal owned by someone else is exactly how parallel shells +// clobber each other's foreground state. +func ShellOwnsTerminal(ttyFd int) bool { + pgid, err := unix.IoctlGetInt(ttyFd, unix.TIOCGPGRP) + return err == nil && pgid == syscall.Getpgrp() +} + +// ShellProcessGroup returns this shell's own process group, the fallback +// hand-back target when the recorded previous foreground group has exited. +func ShellProcessGroup() int { + return syscall.Getpgrp() +} + func DuplicateTerminalHandle(fd int) (int, error) { return unix.Dup(fd) } @@ -305,6 +322,11 @@ func CaptureTerminalMode(fd int) (TerminalModeSnapshot, error) { } func (snapshot *posixTerminalModeSnapshot) Restore() error { + // tcsetattr from a background process group raises SIGTTOU (default action: + // stop). At restore time the terminal may already belong to another group, + // so the same protection used around tcsetpgrp applies here. + restoreSignals := IgnoreSignalsForJobControl() + defer restoreSignals() return term.Restore(snapshot.fd, snapshot.state) } diff --git a/mshell/Pathbin_windows.go b/mshell/Pathbin_windows.go index a09c12d..0deef4a 100644 --- a/mshell/Pathbin_windows.go +++ b/mshell/Pathbin_windows.go @@ -463,6 +463,19 @@ func CanControlTerminal(fd int) bool { return IsTerminal(fd) } +// ShellOwnsTerminal is always true on Windows: the foreground state is an +// in-process Ctrl-C routing marker, not shared kernel terminal ownership, so +// there is no cross-process foreground owner to defer to. +func ShellOwnsTerminal(ttyFd int) bool { + return true +} + +// ShellProcessGroup returns the marker's idle value. Restoring the in-memory +// marker never fails, so the fallback hand-back path is unreachable on Windows. +func ShellProcessGroup() int { + return 0 +} + func DuplicateTerminalHandle(fd int) (int, error) { process := windows.CurrentProcess() var duplicate windows.Handle diff --git a/mshell/ProcessTerminalControl.go b/mshell/ProcessTerminalControl.go index 3b2c568..0766816 100644 --- a/mshell/ProcessTerminalControl.go +++ b/mshell/ProcessTerminalControl.go @@ -1,10 +1,12 @@ package main import ( + "errors" "fmt" "io" "os" "sync" + "syscall" ) // TerminalEndpoint is a resolved terminal used by a child process. The file @@ -128,6 +130,8 @@ type terminalControlBackend interface { setForeground(terminalFd, pgid int) (int, error) restoreForeground(terminalFd, pgid int) error continueProcessGroup(pgid int) error + shellOwnsTerminal(terminalFd int) bool + shellProcessGroup() int } type platformTerminalControlBackend struct{} @@ -154,6 +158,14 @@ func (platformTerminalControlBackend) continueProcessGroup(pgid int) error { return ContinueProcessGroup(pgid) } +func (platformTerminalControlBackend) shellOwnsTerminal(terminalFd int) bool { + return ShellOwnsTerminal(terminalFd) +} + +func (platformTerminalControlBackend) shellProcessGroup() int { + return ShellProcessGroup() +} + // shellInputGate makes the no-competing-reads invariant executable. Today the // interactive reader is synchronous, but keeping this gate at the actual Read // boundary preserves the invariant if input later moves to a worker goroutine. @@ -235,6 +247,17 @@ func (controller *foregroundController) acquire(endpoint *TerminalEndpoint, pgid } controller.mu.Lock() + // Only run the foreground transaction when this shell currently owns the + // terminal, the same gate bash and fish apply. A shell that is not the + // foreground owner (one of several parallel shells sharing a terminal under + // redo or make -j, or a backgrounded script) taking the terminal is exactly + // how the cross-process reclaim race starts. Its child simply runs without + // a handoff; a child that reads the terminal is stopped by SIGTTIN, which is + // standard background-job behavior. + if !controller.backend.shellOwnsTerminal(endpoint.fd) { + controller.mu.Unlock() + return nil, nil + } if err := controller.inputGate.beginForeground(); err != nil { controller.mu.Unlock() return nil, err @@ -249,6 +272,13 @@ func (controller *foregroundController) acquire(endpoint *TerminalEndpoint, pgid if err != nil { controller.inputGate.endForeground() controller.mu.Unlock() + // ESRCH: the child group is already fully reaped (a fast pipeline's + // stages are waited concurrently, so this races with acquisition). + // There is nothing left to foreground and the terminal was not touched; + // run without a handoff instead of killing a job that already finished. + if errors.Is(err, syscall.ESRCH) { + return nil, nil + } return nil, fmt.Errorf("give terminal fd %d to process group %d: %w", endpoint.fd, pgid, err) } @@ -270,6 +300,12 @@ func (controller *foregroundController) acquire(endpoint *TerminalEndpoint, pgid if modeRestoreErr != nil { return nil, fmt.Errorf("continue process group %d: %w; terminal-mode rollback also failed: %v", pgid, err, modeRestoreErr) } + // ESRCH: the group vanished between tcsetpgrp and SIGCONT because its + // members were reaped concurrently. The rollback above already returned + // the terminal; a job that has already finished needs no foregrounding. + if errors.Is(err, syscall.ESRCH) { + return nil, nil + } return nil, fmt.Errorf("continue process group %d: %w", pgid, err) } @@ -290,17 +326,37 @@ func (lease *ForegroundLease) Release() error { } lease.released = true - err := lease.controller.backend.restoreForeground(lease.terminal.fd, lease.previousPgid) + backend := lease.controller.backend + err := backend.restoreForeground(lease.terminal.fd, lease.previousPgid) + if err != nil { + // The recorded previous owner is a snapshot, not a stable handle: under + // a parallel runner it can be a sibling shell's transient child group + // that has already exited, so the hand-back fails with ESRCH. That is + // bookkeeping, not a command failure. Hand the terminal to this shell's + // own group instead, which is the group bash and fish restore. + if fallbackErr := backend.restoreForeground(lease.terminal.fd, backend.shellProcessGroup()); fallbackErr == nil { + err = nil + } else { + err = fmt.Errorf("restore terminal fd %d to process group %d: %w; restore to own process group also failed: %v", lease.terminal.fd, lease.previousPgid, err, fallbackErr) + } + } var modeRestoreErr error if err == nil { modeRestoreErr = lease.modeSnapshot.Restore() - } - if err == nil && modeRestoreErr == nil { + if modeRestoreErr == nil { + lease.controller.inputGate.endForeground() + } + } else { + // Both hand-back targets failed, so the terminal itself is unusable + // (for example a closed PTY). Keeping shell input blocked would wedge + // every later command behind a gate protecting a terminal that no + // longer exists; later commands re-probe the terminal themselves and + // skip control when it is gone. lease.controller.inputGate.endForeground() } lease.controller.mu.Unlock() if err != nil { - lease.releaseErr = fmt.Errorf("restore terminal fd %d to process group %d: %w", lease.terminal.fd, lease.previousPgid, err) + lease.releaseErr = err } else if modeRestoreErr != nil { lease.releaseErr = fmt.Errorf("restore terminal fd %d mode: %w", lease.terminal.fd, modeRestoreErr) } diff --git a/mshell/ProcessTerminalControl_test.go b/mshell/ProcessTerminalControl_test.go index 717ccc7..3b6e549 100644 --- a/mshell/ProcessTerminalControl_test.go +++ b/mshell/ProcessTerminalControl_test.go @@ -5,6 +5,7 @@ import ( "reflect" "strings" "sync" + "syscall" "testing" "time" ) @@ -16,8 +17,11 @@ type fakeTerminalControlBackend struct { captureErr error setErr error continueErr error - restoreErr error + restoreErrs []error // popped once per restoreForeground call; nil entries succeed + restoreTargets []int modeRestoreErr error + notOwner bool + shellPgid int } type fakeTerminalModeSnapshot struct { @@ -51,7 +55,29 @@ func (backend *fakeTerminalControlBackend) setForeground(terminalFd, pgid int) ( func (backend *fakeTerminalControlBackend) restoreForeground(terminalFd, pgid int) error { backend.record("restore") - return backend.restoreErr + backend.mu.Lock() + defer backend.mu.Unlock() + backend.restoreTargets = append(backend.restoreTargets, pgid) + if len(backend.restoreErrs) == 0 { + return nil + } + err := backend.restoreErrs[0] + backend.restoreErrs = backend.restoreErrs[1:] + return err +} + +func (backend *fakeTerminalControlBackend) shellOwnsTerminal(terminalFd int) bool { + return !backend.notOwner +} + +func (backend *fakeTerminalControlBackend) shellProcessGroup() int { + return backend.shellPgid +} + +func (backend *fakeTerminalControlBackend) recordedRestoreTargets() []int { + backend.mu.Lock() + defer backend.mu.Unlock() + return append([]int(nil), backend.restoreTargets...) } func (backend *fakeTerminalControlBackend) continueProcessGroup(pgid int) error { @@ -238,8 +264,83 @@ func TestForegroundControllerRejectsOutstandingShellRead(t *testing.T) { } } -func TestReclaimFailureKeepsShellInputBlocked(t *testing.T) { - backend := &fakeTerminalControlBackend{restoreErr: errors.New("restore failed")} +func TestAcquireSkipsWhenShellDoesNotOwnTerminal(t *testing.T) { + backend := &fakeTerminalControlBackend{notOwner: true} + gate := &shellInputGate{} + controller := foregroundController{backend: backend, inputGate: gate} + + lease, err := controller.acquire(&TerminalEndpoint{fd: 3}, 13) + if err != nil { + t.Fatalf("acquire while not terminal owner returned error: %v", err) + } + if lease != nil { + t.Fatal("acquire while not terminal owner returned a lease") + } + if got := backend.recordedOperations(); len(got) != 0 { + t.Fatalf("backend operations = %v, want none for a non-owner shell", got) + } + // The skipped transaction must leave shell input usable. + if err := gate.beginRead(); err != nil { + t.Fatalf("shell input blocked after skipped acquisition: %v", err) + } + gate.endRead() +} + +func TestAcquireSkipsWhenChildGroupReapedBeforeForeground(t *testing.T) { + backend := &fakeTerminalControlBackend{setErr: syscall.ESRCH} + gate := &shellInputGate{} + controller := foregroundController{backend: backend, inputGate: gate} + + lease, err := controller.acquire(&TerminalEndpoint{fd: 3}, 13) + if err != nil { + t.Fatalf("acquire with a reaped child group returned error: %v", err) + } + if lease != nil { + t.Fatal("acquire with a reaped child group returned a lease") + } + want := []string{"capture", "set"} + if got := backend.recordedOperations(); !reflect.DeepEqual(got, want) { + t.Fatalf("operations = %v, want %v", got, want) + } + if err := gate.beginRead(); err != nil { + t.Fatalf("shell input blocked after skipped acquisition: %v", err) + } + gate.endRead() +} + +func TestAcquireSkipsWhenChildGroupReapedBeforeContinue(t *testing.T) { + backend := &fakeTerminalControlBackend{previousPgid: 7, continueErr: syscall.ESRCH} + gate := &shellInputGate{} + controller := foregroundController{backend: backend, inputGate: gate} + + lease, err := controller.acquire(&TerminalEndpoint{fd: 3}, 13) + if err != nil { + t.Fatalf("acquire with group reaped before SIGCONT returned error: %v", err) + } + if lease != nil { + t.Fatal("acquire with group reaped before SIGCONT returned a lease") + } + // The terminal was handed over before SIGCONT failed, so the rollback + // sequence must still run. + want := []string{"capture", "set", "continue", "restore", "restoreMode"} + if got := backend.recordedOperations(); !reflect.DeepEqual(got, want) { + t.Fatalf("operations = %v, want rollback sequence %v", got, want) + } + if got, want := backend.recordedRestoreTargets(), []int{7}; !reflect.DeepEqual(got, want) { + t.Fatalf("restore targets = %v, want previous group %v", got, want) + } + if err := gate.beginRead(); err != nil { + t.Fatalf("shell input blocked after skipped acquisition: %v", err) + } + gate.endRead() +} + +func TestReleaseFallsBackToOwnGroupWhenPreviousGroupIsGone(t *testing.T) { + backend := &fakeTerminalControlBackend{ + previousPgid: 41, + shellPgid: 77, + restoreErrs: []error{errors.New("no such process")}, + } gate := &shellInputGate{} controller := foregroundController{backend: backend, inputGate: gate} @@ -247,16 +348,48 @@ func TestReclaimFailureKeepsShellInputBlocked(t *testing.T) { if err != nil { t.Fatalf("acquire: %v", err) } - if err := lease.Release(); err == nil { - t.Fatal("release succeeded despite injected restore failure") + if err := lease.Release(); err != nil { + t.Fatalf("release with a dead previous group returned error: %v", err) + } + if got, want := backend.recordedRestoreTargets(), []int{41, 77}; !reflect.DeepEqual(got, want) { + t.Fatalf("restore targets = %v, want dead previous group then own group %v", got, want) + } + want := []string{"capture", "set", "continue", "restore", "restore", "restoreMode"} + if got := backend.recordedOperations(); !reflect.DeepEqual(got, want) { + t.Fatalf("operations = %v, want %v", got, want) + } + if err := gate.beginRead(); err != nil { + t.Fatalf("shell input blocked after successful fallback restore: %v", err) + } + gate.endRead() +} + +func TestReleaseReportsErrorWhenFallbackRestoreAlsoFails(t *testing.T) { + backend := &fakeTerminalControlBackend{ + previousPgid: 41, + shellPgid: 77, + restoreErrs: []error{errors.New("restore failed"), errors.New("terminal gone")}, + } + gate := &shellInputGate{} + controller := foregroundController{backend: backend, inputGate: gate} + + lease, err := controller.acquire(&TerminalEndpoint{fd: 3}, 13) + if err != nil { + t.Fatalf("acquire: %v", err) + } + releaseErr := lease.Release() + if releaseErr == nil || !strings.Contains(releaseErr.Error(), "own process group also failed") { + t.Fatalf("release error = %v, want combined restore failure", releaseErr) } if err := lease.Release(); err == nil { t.Fatal("repeated release hid the prior restoration failure") } - if err := gate.beginRead(); err == nil { - gate.endRead() - t.Fatal("shell input was unblocked after terminal restoration failed") + // Both hand-back targets failing means the terminal itself is unusable, so + // shell input must not stay wedged behind it. + if err := gate.beginRead(); err != nil { + t.Fatalf("shell input blocked after unrecoverable terminal loss: %v", err) } + gate.endRead() } func TestModeRestoreFailureKeepsShellInputBlocked(t *testing.T) { diff --git a/mshell/ProcessTerminalControl_unix_test.go b/mshell/ProcessTerminalControl_unix_test.go index af3a242..c55c627 100644 --- a/mshell/ProcessTerminalControl_unix_test.go +++ b/mshell/ProcessTerminalControl_unix_test.go @@ -146,6 +146,93 @@ func runPipedStdinPTYHelper(t *testing.T, helperName, terminalInput string, expe } } +const parallelReclaimSleepEnv = "MSHELL_PARALLEL_RECLAIM_SLEEP" + +// TestParallelTerminalReclaimHelper runs only inside the shared PTY session +// created by TestParallelShellsSharingPTYSucceed. Each helper is one msh-like +// process running a single external command whose stdio is the shared terminal. +func TestParallelTerminalReclaimHelper(t *testing.T) { + if os.Getenv(terminalHandoffHelperEnv) != "1" { + t.Skip("parallel terminal reclaim helper") + } + duration := os.Getenv(parallelReclaimSleepEnv) + if duration == "" { + duration = "0.2" + } + + list := NewList(2) + list.Items[0] = MShellString{Content: "sleep"} + list.Items[1] = MShellString{Content: duration} + + pbm := NewPathBinManager() + state := &EvalState{} + context := ExecuteContext{ + StandardOutput: os.Stdout, + StandardError: os.Stderr, + Pbm: pbm, + } + + result, exitCode, _, _ := RunProcess(*list, context, state) + if !result.Success || exitCode != 0 { + t.Fatalf("RunProcess result.Success = %v, exitCode = %d", result.Success, exitCode) + } + fmt.Fprintln(os.Stdout, "PARALLEL_OK") +} + +// TestParallelShellsSharingPTYSucceed reproduces the race hit under parallel +// runners such as redo or make -j: several shells share one terminal, each +// wraps its child in a foreground transaction, and the "previous foreground +// process group" each records is a sibling's transient child group. By release +// time that group has exited, so the restoring tcsetpgrp fails with ESRCH. +// Every helper's child exits 0, so every helper must succeed. +func TestParallelShellsSharingPTYSucceed(t *testing.T) { + if testing.Short() { + t.Skip("PTY integration test") + } + + // The staggered durations make earlier siblings' child groups reliably dead + // by the time later helpers hand the terminal back to them. + script := `for d in 0.15 0.3 0.45 0.6 0.75 0.9; do ` + + parallelReclaimSleepEnv + `="$d" "$1" -test.run '^TestParallelTerminalReclaimHelper$' & ` + + `done; wait; printf 'WRAPPER_DONE\n'` + command := exec.Command("sh", "-c", script, "sh", os.Args[0]) + command.Env = append(os.Environ(), terminalHandoffHelperEnv+"=1") + ptmx, err := pty.Start(command) + if err != nil { + t.Fatalf("start parallel helpers in PTY: %v", err) + } + + readDone := make(chan []byte, 1) + go func() { + output, _ := io.ReadAll(ptmx) + readDone <- output + }() + + waitDone := make(chan error, 1) + go func() { + waitDone <- command.Wait() + }() + + select { + case err := <-waitDone: + ptmx.Close() + output := <-readDone + if err != nil { + t.Fatalf("PTY wrapper failed: %v\noutput:\n%s", err, output) + } + if !bytes.Contains(output, []byte("WRAPPER_DONE")) { + t.Fatalf("PTY output does not contain WRAPPER_DONE; output:\n%s", output) + } + if got := bytes.Count(output, []byte("PARALLEL_OK")); got != 6 { + t.Fatalf("PARALLEL_OK count = %d, want 6; output:\n%s", got, output) + } + case <-time.After(30 * time.Second): + terminatePTYProcess(t, command, ptmx) + output := <-readDone + t.Fatalf("parallel reclaim test hung; killed helper process group\noutput:\n%s", output) + } +} + func terminatePTYProcess(t *testing.T, command *exec.Cmd, ptmx *os.File) { t.Helper() if command.Process != nil {