style: bun run fix
This commit is contained in:
roboomp
@@ -4,7 +4,7 @@
|
||||
|
||||
### 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 its own PID instead. Targeting self also leaves long-lived children (MCP stdio servers, the persistent brush native shell) running across the suspend — they're no longer frozen mid-IPC during a quick fg/bg detour ([#3461](https://github.com/can1357/oh-my-pi/issues/3461)).
|
||||
- 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
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -926,35 +926,41 @@ export class InputController {
|
||||
this.ctx.ui.stop();
|
||||
|
||||
try {
|
||||
// SIGSTOP — not SIGTSTP — and to our own PID, not the foreground process
|
||||
// group. Two reasons:
|
||||
// SIGSTOP — not SIGTSTP — to the foreground process group (pid=0).
|
||||
//
|
||||
// 1. 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 — it stops the
|
||||
// process at the kernel regardless of what handlers are installed.
|
||||
// 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.
|
||||
//
|
||||
// 2. Targeting our own PID instead of pgid=0 leaves long-lived children
|
||||
// (MCP stdio servers, the persistent brush native shell) running
|
||||
// while the agent is parked. Real terminal Ctrl+Z reaches the whole
|
||||
// foreground group only because the kernel delivers it there; an
|
||||
// internally-generated suspend has no reason to freeze the agent's
|
||||
// IPC partners mid-request, and the parent shell still detects
|
||||
// WIFSTOPPED via SIGCHLD and parks the job.
|
||||
process.kill(process.pid, "SIGSTOP");
|
||||
// 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) {
|
||||
// The runtime refused the signal (e.g. seccomp filter blocks SIGSTOP
|
||||
// delivery to self). Tear the resume hook down and bring the TUI back
|
||||
// so the user is not stranded on a frozen prompt.
|
||||
// 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);
|
||||
|
||||
@@ -64,7 +64,7 @@ describe("InputController.handleCtrlZ", () => {
|
||||
expect(showError).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("SIGSTOPs our own PID and registers a SIGCONT resume hook on POSIX (#3461)", () => {
|
||||
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(process.pid, "SIGSTOP");
|
||||
expect(killSpy).toHaveBeenCalledWith(0, "SIGSTOP");
|
||||
expect(ui.start).not.toHaveBeenCalled();
|
||||
expect(showError).not.toHaveBeenCalled();
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import * as path from "node:path";
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import * as path from "node:path";
|
||||
|
||||
/**
|
||||
* Regression for https://github.com/can1357/oh-my-pi/issues/3461
|
||||
@@ -14,28 +14,25 @@ import { describe, expect, it } from "bun:test";
|
||||
* action, and `process.kill(0, "SIGTSTP")` from the Ctrl+Z handler became
|
||||
* a no-op.
|
||||
*
|
||||
* The fix sends SIGSTOP (uncatchable) to our own PID instead. 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 side so
|
||||
* a brush upgrade or refactor that removes / gates the SIGTSTP listener
|
||||
* forces a deliberate revisit of `handleCtrlZ` (we could go back to
|
||||
* `process.kill(0, "SIGTSTP")` once the hijack is gone) instead of
|
||||
* silently regressing behavior.
|
||||
* 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 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();
|
||||
@@ -49,14 +46,22 @@ describe("issue #3461 — Ctrl+Z hangs after a command has been run", () => {
|
||||
expect(processesSrc).toContain("tstp_signal_listener");
|
||||
});
|
||||
|
||||
it("handleCtrlZ sends SIGSTOP — not SIGTSTP — to our own PID, defeating the brush hijack", async () => {
|
||||
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();
|
||||
// `process.kill(0, "SIGTSTP")` is the broken call shape; it must not
|
||||
// reappear in the Ctrl+Z handler.
|
||||
// The original broken shape must not return.
|
||||
expect(src).not.toMatch(/process\.kill\(\s*0\s*,\s*["']SIGTSTP["']\s*\)/);
|
||||
// SIGSTOP to our own PID is the only correct shape: SIGSTOP cannot be
|
||||
// caught/blocked/ignored, and targeting self leaves child MCP / native
|
||||
// shell processes running across the suspend.
|
||||
expect(src).toMatch(/process\.kill\(\s*process\.pid\s*,\s*["']SIGSTOP["']\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");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user