diff --git a/src/bun.js/bindings/c-bindings.cpp b/src/bun.js/bindings/c-bindings.cpp index 4d91a42850..8d61988731 100644 --- a/src/bun.js/bindings/c-bindings.cpp +++ b/src/bun.js/bindings/c-bindings.cpp @@ -382,7 +382,9 @@ extern "C" ssize_t pwritev2(int fd, const struct iovec* iov, int iovcnt, extern "C" void Bun__onExit(); extern "C" int32_t bun_stdio_tty[3]; #if !OS(WINDOWS) -static termios termios_to_restore_later[3]; +termios termios_to_restore_later[3]; +// Track which terminal settings were actually saved +bool termios_was_saved[3] = { false, false, false }; #endif extern "C" void bun_restore_stdio() @@ -392,7 +394,7 @@ extern "C" void bun_restore_stdio() // restore stdio for (int32_t fd = 0; fd < 3; fd++) { - if (!bun_stdio_tty[fd]) + if (!bun_stdio_tty[fd] || !termios_was_saved[fd]) continue; sigset_t sa; @@ -459,7 +461,7 @@ extern "C" void bun_initialize_process() #if OS(LINUX) || OS(DARWIN) int devNullFd_ = -1; - bool anyTTYs = false; + // bool anyTTYs = false; // No longer needed since we don't set up signal handlers here const auto setDevNullFd = [&](int target_fd) -> void { bun_is_stdio_null[target_fd] = 1; @@ -494,15 +496,10 @@ extern "C" void bun_initialize_process() } } else { bun_stdio_tty[fd] = 1; - int err = 0; - do { - err = tcgetattr(fd, &termios_to_restore_later[fd]); - } while (err == -1 && errno == EINTR); - - if (err == 0) [[likely]] { - anyTTYs = true; - } + // Following libuv's approach: DO NOT save any terminal settings at initialization + // Only save them when/if we actually modify them via Bun__ttySetMode + // This prevents interfering with programs like 'less' that need terminal control } } @@ -511,18 +508,21 @@ extern "C" void bun_initialize_process() close(devNullFd_); } - // Restore TTY state on exit - if (anyTTYs) { - struct sigaction sa; - memset(&sa, 0, sizeof(sa)); - sigemptyset(&sa.sa_mask); + // Only restore TTY state on exit if we're actually modifying terminal settings + // Don't set up signal handlers just because stdout/stderr are TTYs, as this + // can interfere with programs like 'less' that control stdin when our output is piped + // Signal handlers will be set up by Bun__ttySetMode if/when terminal modes are actually changed + // if (anyTTYs) { + // struct sigaction sa; + // memset(&sa, 0, sizeof(sa)); + // sigemptyset(&sa.sa_mask); - sa.sa_flags = SA_RESETHAND; - sa.sa_handler = onExitSignal; + // sa.sa_flags = SA_RESETHAND; + // sa.sa_handler = onExitSignal; - sigaction(SIGTERM, &sa, nullptr); - sigaction(SIGINT, &sa, nullptr); - } + // sigaction(SIGTERM, &sa, nullptr); + // sigaction(SIGINT, &sa, nullptr); + // } #elif OS(WINDOWS) for (int fd = 0; fd <= 2; ++fd) { auto handle = reinterpret_cast(uv_get_osfhandle(fd)); diff --git a/src/bun.js/bindings/wtf-bindings.cpp b/src/bun.js/bindings/wtf-bindings.cpp index 7d9d6fb10d..01833b9a54 100644 --- a/src/bun.js/bindings/wtf-bindings.cpp +++ b/src/bun.js/bindings/wtf-bindings.cpp @@ -23,6 +23,15 @@ static std::once_flag reset_once_flag; static int current_tty_mode = 0; static struct termios orig_tty_termios; +// External declarations for terminal restoration +extern "C" int32_t bun_stdio_tty[3]; +extern struct termios termios_to_restore_later[3]; +extern bool termios_was_saved[3]; + +#include +extern "C" void onExitSignal(int sig); +static std::once_flag signal_handler_once_flag; + int uv__tcsetattr(int fd, int how, const struct termios* term) { int rc; @@ -124,6 +133,13 @@ extern "C" int Bun__ttySetMode(int fd, int mode) if (orig_termios_fd == -1) { orig_termios = orig_tty_termios; orig_termios_fd = fd; + + // Save the terminal settings for restoration later + // This follows libuv's approach - only save when we actually modify + if (fd >= 0 && fd <= 2 && bun_stdio_tty[fd]) { + termios_to_restore_later[fd] = orig_tty_termios; + termios_was_saved[fd] = true; + } } atomic_store(&orig_termios_spinlock, 0); @@ -146,6 +162,17 @@ extern "C" int Bun__ttySetMode(int fd, int mode) uv_tty_reset_mode(); }); }); + + // Set up signal handlers when we actually modify terminal settings + std::call_once(signal_handler_once_flag, [] { + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESETHAND; + sa.sa_handler = onExitSignal; + sigaction(SIGTERM, &sa, nullptr); + sigaction(SIGINT, &sa, nullptr); + }); break; case 2: // io uv__tty_make_raw(&tmp); @@ -155,6 +182,17 @@ extern "C" int Bun__ttySetMode(int fd, int mode) uv_tty_reset_mode(); }); }); + + // Set up signal handlers when we actually modify terminal settings + std::call_once(signal_handler_once_flag, [] { + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESETHAND; + sa.sa_handler = onExitSignal; + sigaction(SIGTERM, &sa, nullptr); + sigaction(SIGINT, &sa, nullptr); + }); break; } diff --git a/test/regression/issue/22785-less-terminal-mode.test.ts b/test/regression/issue/22785-less-terminal-mode.test.ts new file mode 100644 index 0000000000..6a20943bbc --- /dev/null +++ b/test/regression/issue/22785-less-terminal-mode.test.ts @@ -0,0 +1,85 @@ +import { test, expect } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import { mkdirSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; + +// Test for GitHub issue #22785 +// `bun script.js | less` sometimes causes terminal input to `less` to be line-buffered +test.skip("piping to less should not interfere with terminal mode", async () => { + // This test is skipped by default because: + // 1. It requires the 'less' command to be installed + // 2. It requires an interactive terminal + // 3. The issue is intermittent (~10% occurrence rate) + // + // To run this test manually: + // bun test test/regression/issue/22785-less-terminal-mode.test.ts + + const dir = tmpdir(); + const scriptPath = join(dir, "test-script.js"); + + // Create a script that outputs many lines + const scriptContent = ` + for (let i = 0; i < 1000; i++) { + process.stdout.write(\`\${i}\\n\`); + } + `; + writeFileSync(scriptPath, scriptContent); + + // Test multiple times since the issue is intermittent + const attempts = 50; + let failureCount = 0; + + for (let i = 0; i < attempts; i++) { + // Run bun piped to less and send 'q' to quit + // If the bug occurs, less will require Enter after 'q' + const proc = Bun.spawn({ + cmd: ["sh", "-c", `echo q | ${bunExe()} ${scriptPath} | less`], + env: { ...bunEnv, TERM: "xterm" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr] = await Promise.all([ + proc.stdout.text(), + proc.stderr.text(), + ]); + + // If 'q' appears in stdout, it means less didn't quit immediately + // and displayed the 'q' character (bug occurred) + if (stdout.includes("q")) { + failureCount++; + } + + await proc.exited; + } + + // The bug should not occur with our fix + expect(failureCount).toBe(0); +}); + +// Test that stderr being redirected prevents the issue (as reported) +test.skip("piping to less with stderr redirected should work", async () => { + const dir = tmpdir(); + const scriptPath = join(dir, "test-script.js"); + + const scriptContent = ` + for (let i = 0; i < 100; i++) { + process.stdout.write(\`\${i}\\n\`); + } + `; + writeFileSync(scriptPath, scriptContent); + + // With stderr redirected, the issue should never occur + const proc = Bun.spawn({ + cmd: ["sh", "-c", `echo q | ${bunExe()} ${scriptPath} 2>/dev/null | less`], + env: { ...bunEnv, TERM: "xterm" }, + stdout: "pipe", + }); + + const stdout = await proc.stdout.text(); + await proc.exited; + + // 'q' should not appear in output + expect(stdout).not.toContain("q"); +}); \ No newline at end of file