From 3589f735ee9ad8f2904e773565d76e70def962cd Mon Sep 17 00:00:00 2001 From: xiaolai Date: Sun, 31 May 2026 00:55:33 +0800 Subject: [PATCH] fix(pty): free Rust session on kill() to stop FD leak (#974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kill() removes the pty:exit listener via _cleanup() before pty_kill, so the exit handler that calls pty_close never runs — every terminal close leaked a Rust session map entry (FDs, channels, child handle). Call pty_close explicitly after pty_kill (in finally, so a failed kill still frees the session), and add the same close to the setup-time guard path that also tears down listeners eagerly. Add a regression test asserting pty_close is invoked after kill(). Closes #974 --- src/lib/pty.test.ts | 70 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/pty.ts | 19 +++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 src/lib/pty.test.ts diff --git a/src/lib/pty.test.ts b/src/lib/pty.test.ts new file mode 100644 index 00000000..4fade8be --- /dev/null +++ b/src/lib/pty.test.ts @@ -0,0 +1,70 @@ +// Regression test for #974 — VMarkPty.kill() leaked the Rust session map entry +// because _cleanup() removes the pty:exit listener BEFORE pty_kill, so the +// exit handler that calls pty_close never runs. kill() must call pty_close +// itself (after pty_kill), and the setup-time guard path must too. +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { invokeMock, listenMock } = vi.hoisted(() => ({ + invokeMock: vi.fn(), + listenMock: vi.fn(), +})); + +vi.mock("@tauri-apps/api/core", () => ({ + invoke: (...args: unknown[]) => invokeMock(...args), +})); +vi.mock("@tauri-apps/api/event", () => ({ + listen: (...args: unknown[]) => listenMock(...args), +})); +vi.mock("@/utils/debug", () => ({ ptyWarn: vi.fn(), terminalLog: vi.fn() })); + +// The global test setup (src/test/setup.ts) mocks the whole @/lib/pty module +// for Terminal component tests. This suite exercises the REAL implementation, +// so unmock it here (the Tauri core/event mocks above still apply). +vi.unmock("@/lib/pty"); + +import { spawn } from "@/lib/pty"; + +const PID = 4242; + +beforeEach(() => { + invokeMock.mockReset(); + listenMock.mockReset(); + invokeMock.mockImplementation((cmd: string) => + cmd === "pty_spawn" ? Promise.resolve(PID) : Promise.resolve(undefined), + ); + // listen() resolves to an unlisten fn + listenMock.mockResolvedValue(() => {}); +}); + +describe("VMarkPty.kill() — #974 session leak", () => { + it("calls pty_close after pty_kill so the Rust session is freed", async () => { + const pty = spawn("bash", []); + // Wait for setup to finish (reader started = ready). + await vi.waitFor(() => { + expect(invokeMock).toHaveBeenCalledWith("pty_start", { pid: PID }); + }); + + pty.kill(); + + await vi.waitFor(() => { + expect(invokeMock).toHaveBeenCalledWith("pty_close", { pid: PID }); + }); + expect(invokeMock).toHaveBeenCalledWith("pty_kill", { pid: PID }); + // Exactly once on the normal path (the eagerly-removed exit listener can't + // double-close). + const closeCalls = invokeMock.mock.calls.filter((c) => c[0] === "pty_close"); + expect(closeCalls).toHaveLength(1); + }); + + it("frees the session even when kill() races setup (setup-guard path)", async () => { + const pty = spawn("bash", []); + pty.kill(); // before _ready resolves → hits the setup guard + + await vi.waitFor(() => { + expect(invokeMock).toHaveBeenCalledWith("pty_close", { pid: PID }); + }); + expect(invokeMock).toHaveBeenCalledWith("pty_kill", { pid: PID }); + // Guard aborts before phase 3 — the reader never starts. + expect(invokeMock).not.toHaveBeenCalledWith("pty_start", { pid: PID }); + }); +}); diff --git a/src/lib/pty.ts b/src/lib/pty.ts index 3f1bb288..417d2d62 100644 --- a/src/lib/pty.ts +++ b/src/lib/pty.ts @@ -16,7 +16,10 @@ * Uint8Array. * - `kill()` eagerly cleans up event listeners and guards against mid-setup * races via a `_destroyed` flag. - * - `pty_close` is called after exit to free the Rust-side session (FDs). + * - `pty_close` frees the Rust-side session (FDs/channels/child handle). It + * runs from the exit handler on natural exit; but because `kill()` (and the + * mid-setup guard) tear down the exit listener first, those paths call + * `pty_close` directly so the session is never leaked (#974). * * @coordinates-with src-tauri/src/pty.rs — Rust backend commands and events * @coordinates-with components/Terminal/spawnPty.ts — consumes this wrapper @@ -162,6 +165,11 @@ class VMarkPty implements IPty { await invoke("pty_kill", { pid: this._pid }).catch((err) => { terminalLog("pty_kill (setup guard) failed:", errorMessage(err)); }); + // Listeners were torn down, so the exit handler won't free the session + // (#974). Close it explicitly here too. + await invoke("pty_close", { pid: this._pid }).catch((err) => { + terminalLog("pty_close (setup guard) failed:", errorMessage(err)); + }); return; } @@ -197,10 +205,19 @@ class VMarkPty implements IPty { kill(): void { this._destroyed = true; this._cleanup(); + // _cleanup() removed the pty:exit listener, so the natural exit handler + // that calls pty_close never runs (#974). Close the Rust session + // explicitly after pty_kill — in finally so a failed kill still frees the + // session map entry (FDs, channels, child handle). this._ready .then(() => invoke("pty_kill", { pid: this._pid })) .catch((err) => { terminalLog("pty_kill failed:", errorMessage(err)); + }) + .finally(() => { + invoke("pty_close", { pid: this._pid }).catch((err) => { + terminalLog("pty_close failed:", errorMessage(err)); + }); }); }