terminal: take the reader's ref before the reader starts

src/runtime/api/bun/Terminal.rs

@@ -473,23 +473,25 @@ impl Terminal {
             }
         }
 
-        // Start reader with the read fd - adds a ref
+        // Start reader with the read fd. The reader's ref is taken first: when
+        // the poll registration fails, POSIX `start()` calls `on_reader_error`,
+        // which releases that ref, and still returns Ok.
+        terminal.ref_();
         match terminal
             .reader
             .with_mut(|r| r.start(pty_result.read_fd, true))
         {
             sys::Result::Err(_) => {
-                // Reader never started: closeInternal skips reader.close() but
-                // runs writer.close() → onWriterClose → deref (2→1). Then drop
-                // the initial ref (1→0).
+                // No callback ran: the reader took neither read_fd nor its ref.
                 terminal.read_fd.get().close();
                 terminal.read_fd.set(Fd::INVALID);
-                terminal.close_internal();
                 terminal.deref_();
-                return Err(InitError::ReaderStartFailed);
+                return Err(terminal.fail_reader_start());
+            }
+            sys::Result::Ok(()) if terminal.flags.get().contains(Flags::READER_DONE) => {
+                return Err(terminal.fail_reader_start());
             }
             sys::Result::Ok(()) => {
-                terminal.ref_();
                 #[cfg(unix)]
                 {
                     terminal.reader.with_mut(|r| {
@@ -509,6 +511,10 @@ impl Terminal {
         // SAFETY: the reader cell is live for the terminal's lifetime; `read`
         // is the raw re-entrancy-safe entry (its dispatch runs user JS).
         unsafe { IOReader::read(terminal.reader.as_ptr()) };
+        // The first read can end the reader too: EOF, a read error, or a failed re-arm.
+        if terminal.flags.get().contains(Flags::READER_DONE) {
+            return Err(terminal.fail_reader_start());
+        }
 
         // Get or create the JS wrapper
         let this_value = existing_js_value.unwrap_or_else(|| js::to_js(parent_ptr, global_object));
@@ -539,6 +545,17 @@ impl Terminal {
         })
     }
 
+    /// `init_terminal` error path for a reader that finished before the
+    /// terminal reached JS, with the reader's ref already released.
+    /// `close_internal` closes what is still open (a writer that is still
+    /// open releases its ref through `on_writer_close`), then the initial ref
+    /// is dropped, which may free `self`.
+    fn fail_reader_start(&self) -> InitError {
+        self.close_internal();
+        self.deref_();
+        InitError::ReaderStartFailed
+    }
+
     /// Constructor for Terminal - called from JavaScript
     /// With constructNeedsThis: true, we receive the JSValue wrapper directly.
     /// Thunk emitted by `.classes.ts` codegen (`TerminalClass__construct` in

test/js/bun/spawn/spawn-pipe-start-error.test.ts

@@ -81,26 +81,45 @@ try {
 // EPOLL_CTL_ADD asking for writability with ENOSPC (what an exhausted
 // fs.epoll.max_user_watches returns). Readable registrations, and uSockets,
 // which uses the wrapper, are unaffected.
+//
+// FAIL_EPOLL_CTL=pty-reader-add or pty-reader-mod fails one readable
+// registration of a pty master instead, and nothing else: the EPOLL_CTL_ADD
+// that Bun.Terminal's reader makes when it starts, or the EPOLL_CTL_MOD that
+// re-arms it after the first read. The kernel fails the ADD when watches or
+// memory run out. It does not fail the MOD that way: that mode only stands in
+// for any error that ends the reader during the constructor's first read.
 const cc = Bun.which("cc") || Bun.which("gcc") || Bun.which("clang");
 
 const SHIM_C = /* c */ `
 #define _GNU_SOURCE
 #include <dlfcn.h>
 #include <errno.h>
 #include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
 #include <sys/epoll.h>
+#include <sys/ioctl.h>
 #include <sys/syscall.h>
 
 static long (*real_syscall)(long, ...);
 
+static int should_fail(long op, int fd, struct epoll_event *event) {
+  if (!event) return 0;
+  const char *mode = getenv("FAIL_EPOLL_CTL");
+  if (!mode) return op == EPOLL_CTL_ADD && (event->events & EPOLLOUT);
+  long failing_op = strcmp(mode, "pty-reader-add") == 0 ? EPOLL_CTL_ADD : EPOLL_CTL_MOD;
+  unsigned int pty_number;
+  // TIOCGPTN succeeds on a pty master only.
+  return op == failing_op && (event->events & EPOLLIN) && ioctl(fd, TIOCGPTN, &pty_number) == 0;
+}
+
 long syscall(long number, ...) {
   va_list ap;
   va_start(ap, number);
   long a1 = va_arg(ap, long), a2 = va_arg(ap, long), a3 = va_arg(ap, long);
   long a4 = va_arg(ap, long), a5 = va_arg(ap, long), a6 = va_arg(ap, long);
   va_end(ap);
-  if (number == SYS_epoll_ctl && a2 == EPOLL_CTL_ADD && a4 != 0 &&
-      (((struct epoll_event *)a4)->events & EPOLLOUT)) {
+  if (number == SYS_epoll_ctl && should_fail(a2, (int)a3, (struct epoll_event *)a4)) {
     errno = ENOSPC;
     return -1;
   }
@@ -128,11 +147,14 @@ const wrappers = () => {
 
 // Parked on globalThis so the baseline keeps counting it: a local that is never
 // read again is not kept alive across the awaits below.
-if (kind === "terminal") {
-  globalThis.anchor = Bun.Terminal.prototype;
-} else {
-  globalThis.anchor = Bun.spawn({ cmd: ["true"], stdin: "ignore", stdout: "ignore", stderr: "ignore" });
-  await globalThis.anchor.exited;
+globalThis.anchor = [];
+if (kind.includes("terminal")) {
+  globalThis.anchor.push(Bun.Terminal.prototype);
+}
+if (kind !== "terminal") {
+  const child = Bun.spawn({ cmd: ["true"], stdin: "ignore", stdout: "ignore", stderr: "ignore" });
+  globalThis.anchor.push(child);
+  await child.exited;
 }
 const fdBaseline = openFds();
 const wrapperBaseline = wrappers();
@@ -149,6 +171,9 @@ try {
     case "terminal":
       new Bun.Terminal({});
       break;
+    case "spawn-terminal":
+      Bun.spawn({ cmd: ["true"], terminal: {} });
+      break;
   }
 } catch (e) {
   error = { code: e.code, message: e.message };
@@ -161,43 +186,44 @@ while ((openFds() > fdBaseline || wrappers() > wrapperBaseline) && performance.n
 console.log(JSON.stringify({ error, leakedFds: openFds() - fdBaseline, leakedWrappers: wrappers() - wrapperBaseline }));
 `;
 
-describe.skipIf(!isLinux || !cc)(
-  "a pipe writer whose event loop registration fails leaves its fd to the caller",
-  () => {
-    let dir: ReturnType<typeof tempDir>;
-
-    beforeAll(async () => {
-      dir = tempDir("writer-start-error", { "shim.c": SHIM_C, "fixture.js": FIXTURE });
-      await using ccProc = Bun.spawn({
-        cmd: [cc!, "-shared", "-fPIC", "-o", join(String(dir), "shim.so"), join(String(dir), "shim.c"), "-ldl"],
-        env: bunEnv,
-        stdout: "pipe",
-        stderr: "pipe",
-      });
-      const [ccOut, ccErr, ccExit] = await Promise.all([ccProc.stdout.text(), ccProc.stderr.text(), ccProc.exited]);
-      if (ccExit !== 0) throw new Error(`shim compile failed: ${ccErr || ccOut}`);
-    });
+let dir: ReturnType<typeof tempDir> | undefined;
 
-    afterAll(() => {
-      dir?.[Symbol.dispose]();
-    });
+beforeAll(async () => {
+  if (!isLinux || !cc) return;
+  dir = tempDir("poll-start-error", { "shim.c": SHIM_C, "fixture.js": FIXTURE });
+  await using ccProc = Bun.spawn({
+    cmd: [cc, "-shared", "-fPIC", "-o", join(String(dir), "shim.so"), join(String(dir), "shim.c"), "-ldl"],
+    env: bunEnv,
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [ccOut, ccErr, ccExit] = await Promise.all([ccProc.stdout.text(), ccProc.stderr.text(), ccProc.exited]);
+  if (ccExit !== 0) throw new Error(`shim compile failed: ${ccErr || ccOut}`);
+});
 
-    async function runFixture(kind: string, env: Record<string, string> = {}) {
-      await using proc = Bun.spawn({
-        cmd: [bunExe(), "fixture.js", kind],
-        cwd: String(dir),
-        env: { ...bunEnv, ...env, LD_PRELOAD: join(String(dir), "shim.so") },
-        stdout: "pipe",
-        stderr: "pipe",
-      });
-      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
-      let report: unknown = stdout;
-      try {
-        report = JSON.parse(stdout);
-      } catch {}
-      return { report, stderr, exitCode };
-    }
+afterAll(() => {
+  dir?.[Symbol.dispose]();
+});
 
+async function runFixture(kind: string, env: Record<string, string> = {}) {
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "fixture.js", kind],
+    cwd: String(dir),
+    env: { ...bunEnv, ...env, LD_PRELOAD: join(String(dir), "shim.so") },
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  let report: unknown = stdout;
+  try {
+    report = JSON.parse(stdout);
+  } catch {}
+  return { report, stderr, exitCode };
+}
+
+describe.skipIf(!isLinux || !cc)(
+  "a pipe writer whose event loop registration fails leaves its fd to the caller",
+  () => {
     test.concurrent("Bun.spawn with stdin: 'pipe' closes the stdin pipe exactly once", async () => {
       expect(await runFixture("stdin-pipe")).toEqual({
         // The spawn bindings report a failed stdin setup generically, so only
@@ -235,3 +261,25 @@ describe.skipIf(!isLinux || !cc)(
     });
   },
 );
+
+// The POSIX reader reports a failed registration by calling on_reader_error
+// from inside start(), and start() still returns Ok. Terminal's reader
+// callbacks end by releasing the reader's ref, which the constructor used to
+// take only after start() returned: the release freed the Terminal while the
+// constructor was still using it. A reader that ends during the constructor's
+// first read (the injected EPOLL_CTL_MOD failure) did not free early. It left
+// a constructed Terminal that was already dead and could never be collected.
+describe.skipIf(!isLinux || !cc)("a Bun.Terminal whose reader fails to register with the event loop", () => {
+  describe.each(["pty-reader-add", "pty-reader-mod"])("FAIL_EPOLL_CTL=%s", mode => {
+    test.concurrent.each([
+      ["new Bun.Terminal()", "terminal"],
+      ["Bun.spawn() with terminal options", "spawn-terminal"],
+    ])("%s throws and releases the pty", async (_, kind) => {
+      expect(await runFixture(kind, { FAIL_EPOLL_CTL: mode })).toEqual({
+        report: { error: { message: "Failed to start terminal reader" }, leakedFds: 0, leakedWrappers: 0 },
+        stderr: "",
+        exitCode: 0,
+      });
+    });
+  });
+});