From 4fb186671bde73fee5874d75fedde2fda0821105 Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Wed, 24 Jun 2026 08:44:56 -0400 Subject: [PATCH] fix(pane-stream): release FIFO read fd on pane WebSocket disconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PaneStreamer.run() transfers the FIFO read fd into an asyncio read transport via connect_read_pipe and sets the bare `fd` to -1, but the finally block only guarded that now-dead `fd` and never closed the transport. The transport only self-closes on EOF — and on a WS disconnect the tmux `cat` writer is still alive, so no EOF arrives and the read fd leaks one-per-stream (the .fifo is unlinked but the fd stays open). Over a long session of opening pane terminals this marches toward the process fd limit and bloats the asyncio selector, slowing every backend operation including the keystroke round-trip. Capture the transport and close it in finally (with file_obj / bare-fd fallbacks for the earlier failure windows). transport.close() releases the fd and deregisters the loop reader. Verified live: was +1 FIFO fd per terminal open; now 0 leaked across clean and worst-case mid-stream open/close cycles. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/switchboard/services/pane_stream.py | 23 +++++- backend/tests/test_pane_stream.py | 74 +++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/backend/src/switchboard/services/pane_stream.py b/backend/src/switchboard/services/pane_stream.py index 7e0164e..dfdded5 100644 --- a/backend/src/switchboard/services/pane_stream.py +++ b/backend/src/switchboard/services/pane_stream.py @@ -22,7 +22,7 @@ import stat import tempfile import uuid -from typing import TYPE_CHECKING +from typing import IO, TYPE_CHECKING from switchboard.services import claude_parser, tmux @@ -206,6 +206,8 @@ async def run(self) -> None: fd = -1 pipe_active = False prompt_task: asyncio.Task[None] | None = None + file_obj: IO[bytes] | None = None + transport: asyncio.ReadTransport | None = None try: # Open the read end non-blocking so we don't deadlock waiting for a writer. fd = os.open(fifo_path, os.O_RDONLY | os.O_NONBLOCK) @@ -232,7 +234,9 @@ async def run(self) -> None: # connect_read_pipe takes ownership of the fd via the file object. file_obj = os.fdopen(fd, "rb", buffering=0) fd = -1 # ownership transferred - await loop.connect_read_pipe(lambda: asyncio.StreamReaderProtocol(reader), file_obj) + transport, _ = await loop.connect_read_pipe( + lambda: asyncio.StreamReaderProtocol(reader), file_obj + ) # Buffer for partial `ESC k …` sequences that span FIFO reads. title_pending = b"" @@ -261,8 +265,19 @@ async def run(self) -> None: if pipe_active: with contextlib.suppress(Exception): srv.cmd("pipe-pane", "-t", target) # ty: ignore - # Close fd if we still own it. - if fd >= 0: + # Release the FIFO read side. Once connect_read_pipe succeeds the fd + # is owned by the asyncio transport, which only self-closes on EOF — + # and on a WS disconnect the tmux `cat` writer is still alive, so no + # EOF arrives. We MUST close the transport (or, if we never got that + # far, the file object / bare fd) or the read fd leaks one-per-stream + # (the `.fifo` is unlinked below but the fd stays open). transport + # close also deregisters the loop reader. + if transport is not None: + transport.close() + elif file_obj is not None: + with contextlib.suppress(OSError): + file_obj.close() + elif fd >= 0: with contextlib.suppress(OSError): os.close(fd) with contextlib.suppress(OSError): diff --git a/backend/tests/test_pane_stream.py b/backend/tests/test_pane_stream.py index 19c2ff4..4e7f18b 100644 --- a/backend/tests/test_pane_stream.py +++ b/backend/tests/test_pane_stream.py @@ -187,6 +187,80 @@ def test_install_fifo_cleanup_hook_is_idempotent(monkeypatch) -> None: assert pane_stream._atexit_hooked is True +# --- FIFO read-fd release on disconnect (FD-leak regression) ---------------- + + +def _count_open_fds() -> int: + """Open file descriptors for this process. `/dev/fd` is present on both + macOS and Linux; listing it is symmetric across the before/after samples so + any transient fd the listing itself uses cancels out.""" + return len(os.listdir("/dev/fd")) + + +def test_run_releases_fifo_read_fd_on_ws_disconnect(monkeypatch, tmp_path) -> None: + """Regression: a pane stream torn down by a WebSocket disconnect must + release its FIFO read fd. + + `run()` transfers the read fd into an asyncio read transport via + `connect_read_pipe` (and sets the bare `fd` to -1). Before the fix the + `finally` block only guarded the now-dead `fd` and never closed that + transport, so on disconnect — when the tmux `cat` writer is still alive and + no EOF arrives — the read fd leaked (one per connection; the `.fifo` is + unlinked but the fd stays open). This test models that exact path: the + FIFO writer is held open throughout, so the read fd can only be released by + an explicit transport close in teardown. + """ + monkeypatch.setattr(pane_stream, "_FIFO_DIR", str(tmp_path)) + + writer = {"fd": None} + + class _FakeSrv: + def cmd(self, *args: object) -> None: + # `pipe-pane -O -t TARGET 'cat > '` installs the writer. + # Emulate tmux's `cat` by opening the FIFO write side and KEEPING it + # open (no EOF), then priming one chunk so the read loop advances. + # The toggle-off form (`pipe-pane -t TARGET`) is intentionally a + # no-op: we keep the writer alive to model the no-EOF disconnect. + if len(args) >= 5 and args[0] == "pipe-pane": + path = str(args[-1]).split("> ", 1)[1].strip() + writer["fd"] = os.open(path, os.O_WRONLY) + os.write(writer["fd"], b"hello world\r\n") + + class _DisconnectWS: + async def send_text(self, text: str) -> None: # snapshot is empty; unused + raise AssertionError("snapshot should be empty in this test") + + async def send_bytes(self, data: bytes) -> None: + raise ConnectionError("client disconnected") + + monkeypatch.setattr(pane_stream.tmux, "capture_pane", lambda *a, **k: []) + monkeypatch.setattr(pane_stream.tmux, "pane_kind", lambda *a, **k: "shell") + monkeypatch.setattr(pane_stream.tmux, "get_server", lambda: _FakeSrv()) + + async def _run() -> None: + before = _count_open_fds() + streamer = PaneStreamer(session="s", index=0, ws=_DisconnectWS()) + await streamer.run() + # `transport.close()` removes the loop reader synchronously but closes + # the pipe fd via a scheduled callback — drain a few loop iterations so + # that runs. (With the bug present nothing is scheduled, so draining + # frees nothing and the assertion still fails.) + for _ in range(3): + await asyncio.sleep(0) + # Release the writer we held open to model the live tmux `cat`, so the + # only fd that could survive teardown is a leaked read fd. No `await` + # between here and the measurement, so the buggy path can't sneak in a + # late EOF-driven close. + if writer["fd"] is not None: + os.close(writer["fd"]) + after = _count_open_fds() + assert after == before, f"leaked {after - before} fd(s) on disconnect" + # The FIFO file itself must also be unlinked (already handled today). + assert not list(tmp_path.glob("sb-pane-*.fifo")) + + asyncio.run(_run()) + + # --------------------------------------------------------------------------- # Screen/tmux title-set stripping (`ESC k … ESC \`) # ---------------------------------------------------------------------------