smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #3463 into sweep

Browse Source
This commit is contained in:
can1357
2026-06-25 18:53:12 +02:00
parent 451af61280 184f6dd809
commit 6890b0ffd7
5 changed files with 122 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed Ctrl+Z hanging the terminal after any tool call had run: the TUI tore down (`ui.stop()`) but the process kept running in `Sl+` state, leaving the user with a dead terminal recoverable only via `kill -9`. The embedded `brush-core` shell behind every bash tool call installs a tokio SIGTSTP listener on `Process::wait` (`crates/brush-core-vendored/src/sys/unix/signal.rs::tstp_signal_listener` → `tokio::signal::unix::signal(SIGTSTP)`); per tokio's contract, the first call for a SignalKind permanently replaces the kernel-default handler for the lifetime of the process. So the first bash invocation — even `/usr/bin/true` — silently overrode SIGTSTP's "stop" default, and `InputController.handleCtrlZ`'s subsequent `process.kill(0, "SIGTSTP")` was swallowed by tokio. The handler now sends `SIGSTOP` (uncatchable, unblockable, unignorable) to the foreground process group, so the kernel parks omp regardless of installed handlers and the shell sees the whole job stop even when omp runs behind a wrapper (`npx`, `pnpm exec`, `bunx`, …) or as one stage of a pipeline. MCP stdio servers now spawn detached into their own session — they're insulated both from terminal job-control signals (which used to stop their process trees and leave the JSONL read loop blocked on silent pipes) and from the new pgid=0 suspend itself ([#3461](https://github.com/can1357/oh-my-pi/issues/3461)).
## [16.1.19] - 2026-06-25
### Fixed
packages/coding-agent/src/mcp/transports/stdio.ts
+5
View File
@@ -340,6 +340,10 @@ export class StdioTransport implements MCPTransport {
platform: process.platform,
});
// Spawn in a new session (detached → setsid) so the MCP process tree has
// no controlling terminal. Otherwise terminal job-control signals (Ctrl+Z
// SIGTSTP, background-read SIGTTIN) can stop stdio servers such as
// chrome-devtools-mcp and leave our read loop blocked on silent pipes.
this.#process = spawn({
cmd: spawnCommand.cmd,
cwd,
@@ -348,6 +352,7 @@ export class StdioTransport implements MCPTransport {
stdout: "pipe",
stderr: "pipe",
windowsHide: spawnCommand.windowsHide,
detached: true,
});
this.#connected = true;
packages/coding-agent/src/modes/controllers/input-controller.ts
+42 -15
View File
@@ -902,19 +902,19 @@ export class InputController {
}
handleCtrlZ(): void {
// SIGTSTP is POSIX job-control: Windows has no equivalent and
// `process.kill(_, "SIGTSTP")` throws `TypeError: Unknown signal:
// SIGTSTP` there, taking the whole agent down via an uncaught
// exception (issue #2036). No-op on platforms that cannot suspend.
// Job-control suspend is POSIX-only: on Windows `process.kill(_, "SIGSTOP")`
// throws `TypeError: Unknown signal: SIGSTOP` and takes the whole agent down
// via an uncaught exception (issue #2036, originally for SIGTSTP — same
// shape for SIGSTOP). No-op on platforms that cannot suspend.
if (process.platform === "win32") {
this.ctx.showStatus("Suspend (Ctrl+Z) is not supported on this platform");
return;
}
// Capture the listener so we can detach it if the signal never
// fires; otherwise a failed suspend would leave a stale SIGCONT
// handler that fires on the next unrelated continue and tries to
// re-`start()` an already-running TUI.
// Capture the listener so we can detach it if the signal never fires;
// otherwise a failed suspend would leave a stale SIGCONT handler that
// fires on the next unrelated continue and tries to re-`start()` an
// already-running TUI.
const onResume = (): void => {
this.ctx.ui.start();
this.ctx.ui.requestRender(true);
@@ -926,14 +926,41 @@ export class InputController {
this.ctx.ui.stop();
try {
// pid=0 → entire foreground process group; the shell receives
// SIGTSTP and parks the job.
process.kill(0, "SIGTSTP");
// SIGSTOP — not SIGTSTP — to the foreground process group (pid=0).
//
// SIGTSTP: brush-core (the embedded shell behind every bash tool call)
// installs a tokio SIGTSTP listener on `Process::wait` to detect when
// its children have been stopped (`crates/brush-core-vendored/src/sys/
// unix/signal.rs::tstp_signal_listener` → `tokio::signal::unix::
// signal(SIGTSTP)`). Per tokio's documented contract, the first call
// for a given SignalKind permanently replaces the kernel-default
// handler for the lifetime of the process. So once the user has
// issued even one bash command — e.g. `/usr/bin/true` — SIGTSTP no
// longer stops omp: tokio swallows it and the TUI ends up torn down
// while the process keeps running with no live terminal (issue
// [#3461]). SIGSTOP cannot be caught, blocked, or ignored, so the
// kernel stops the process regardless of installed handlers.
//
// pid=0 (foreground process group, not just our PID): omp is not
// always the shell's direct child. Package-manager launchers (`npx`,
// `pnpm exec`, `bunx`, …) wait on the real CLI from a parent shim
// that shares omp's process group, and a `omp … | tee log` style
// pipeline puts a sibling foreground job member in the same group
// too. The shell sees the job as stopped only when its direct
// child / pipeline leader is stopped, so suspending only our PID
// leaves wrappers and pipeline peers running and the terminal
// hung — exactly the failure shape we're fixing. Stopping the whole
// group keeps the shell's job-control view consistent. Long-lived
// children that must survive the suspend (MCP stdio servers via
// the `detached: true` spawn in `mcp/transports/stdio.ts`, every
// brush external command via brush's per-child `setsid` in
// `crates/brush-core-vendored/src/commands.rs`) are already in
// their own sessions, so pgid=0 does not reach them.
process.kill(0, "SIGSTOP");
} catch (err) {
// Either the runtime refused the signal or the kernel rejected
// it (some sandboxes block sending to pid=0). Tear the resume
// hook down and bring the TUI back so the user is not stranded
// on a frozen prompt.
// The runtime refused the signal (e.g. seccomp filter blocks SIGSTOP
// delivery to the process group). Tear the resume hook down and
// bring the TUI back so the user is not stranded on a frozen prompt.
process.removeListener("SIGCONT", onResume);
this.ctx.ui.start();
this.ctx.ui.requestRender(true);
packages/coding-agent/test/input-controller-suspend.test.ts
+4 -4
View File
@@ -44,7 +44,7 @@ afterEach(() => {
});
describe("InputController.handleCtrlZ", () => {
it("no-ops on Windows so the unsupported SIGTSTP signal can't crash the process (#2036)", () => {
it("no-ops on Windows so the unsupported SIGSTOP signal can't crash the process (#2036)", () => {
setPlatform("win32");
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => {
throw new Error("process.kill must not be called on win32");
@@ -64,7 +64,7 @@ describe("InputController.handleCtrlZ", () => {
expect(showError).not.toHaveBeenCalled();
});
it("sends SIGTSTP to the process group and registers a SIGCONT resume hook on POSIX", () => {
it("SIGSTOPs the foreground process group and registers a SIGCONT resume hook on POSIX (#3461)", () => {
setPlatform("linux");
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
const onceSpy = vi.spyOn(process, "once");
@@ -83,7 +83,7 @@ describe("InputController.handleCtrlZ", () => {
expect(stopOrder).toBeLessThan(killOrder);
expect(killSpy).toHaveBeenCalledTimes(1);
expect(killSpy).toHaveBeenCalledWith(0, "SIGTSTP");
expect(killSpy).toHaveBeenCalledWith(0, "SIGSTOP");
expect(ui.start).not.toHaveBeenCalled();
expect(showError).not.toHaveBeenCalled();
@@ -98,7 +98,7 @@ describe("InputController.handleCtrlZ", () => {
it("restores the TUI and drops the SIGCONT listener when process.kill rejects the signal", () => {
setPlatform("linux");
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => {
throw new Error("Unknown signal: SIGTSTP");
throw new Error("Unknown signal: SIGSTOP");
});
const onceSpy = vi.spyOn(process, "once");
const removeSpy = vi.spyOn(process, "removeListener");
packages/coding-agent/test/issue-3461-repro.test.ts
+67
View File
@@ -0,0 +1,67 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
/**
* Regression for https://github.com/can1357/oh-my-pi/issues/3461
*
* Ctrl+Z stopped working after any tool call: the TUI tore down but the
* process kept running (`Sl+`, not `T`), wedging the terminal until
* `kill -9`. Root cause: brush-core's `Process::wait` calls
* `tokio::signal::unix::signal(SIGTSTP)` to detect when its children get
* stopped. Per tokio's documented contract the first call for a SignalKind
* permanently replaces the kernel-default handler — so after the first
* `Shell::run` the parent's SIGTSTP no longer triggers the kernel STOP
* action, and `process.kill(0, "SIGTSTP")` from the Ctrl+Z handler became
* a no-op.
*
* The fix has two halves:
*
* - `handleCtrlZ` sends SIGSTOP (uncatchable) to the foreground process
* group, so the kernel parks omp regardless of installed handlers and the
* parent shell sees the whole job stop even when omp runs behind a wrapper
* (`npx`, `pnpm exec`, `bunx`, …) or as one stage of a pipeline.
* - MCP stdio servers spawn detached, so terminal job-control signals cannot
* stop their process trees and leave the JSONL read loop blocked on silent
* pipes — and so the pgid=0 suspend above doesn't reach them either.
* The unit test in `input-controller-suspend.test.ts` covers the JS handler's
* call shape; this file pins the runtime contract on the brush/MCP side so
* refactors force a deliberate revisit instead of silently regressing behavior.
*/
describe("issue #3461 — Ctrl+Z hangs after a command has been run", () => {
const packageDir = path.resolve(import.meta.dir, "..");
const brushUnixSignal = path.resolve(packageDir, "../../crates/brush-core-vendored/src/sys/unix/signal.rs");
const brushProcesses = path.resolve(packageDir, "../../crates/brush-core-vendored/src/processes.rs");
const inputController = path.resolve(packageDir, "src/modes/controllers/input-controller.ts");
const mcpStdioTransport = path.resolve(packageDir, "src/mcp/transports/stdio.ts");
it("brush-core installs a tokio SIGTSTP listener on every Process::wait", async () => {
const signalSrc = await Bun.file(brushUnixSignal).text();
expect(signalSrc).toContain("tstp_signal_listener");
expect(signalSrc).toContain("tokio::signal::unix::signal");
// Pin the SIGTSTP constant specifically. A move to a non-job-control
// signal would invalidate the assumption this fix is built on.
expect(signalSrc).toMatch(/nix::libc::SIGTSTP/);
const processesSrc = await Bun.file(brushProcesses).text();
expect(processesSrc).toContain("tstp_signal_listener");
});
it("handleCtrlZ sends SIGSTOP to the foreground process group, defeating the brush hijack and covering wrappers/pipelines", async () => {
const src = await Bun.file(inputController).text();
// The original broken shape must not return.
expect(src).not.toMatch(/process\.kill\(\s*0\s*,\s*["']SIGTSTP["']\s*\)/);
// Self-only SIGSTOP was the v1 of this fix; it leaves wrapper/pipeline
// peers in the same process group running and the shell never sees the
// job stop. The handler now targets pgid=0.
expect(src).not.toMatch(/process\.kill\(\s*process\.pid\s*,\s*["']SIGSTOP["']\s*\)/);
// pgid=0 SIGSTOP is the only correct shape: uncatchable, and reaches
// every process the shell considers part of the foreground job.
expect(src).toMatch(/process\.kill\(\s*0\s*,\s*["']SIGSTOP["']\s*\)/);
});
it("MCP stdio servers spawn detached so terminal job-control signals cannot stop them", async () => {
const src = await Bun.file(mcpStdioTransport).text();
expect(src).toMatch(/detached:\s*true/);
expect(src).toContain("no controlling terminal");
});
});