fix: preserve inherited descriptors in launcher

Author: schickling-assistant

bin/pty

@@ -1,16 +1,14 @@
 #!/usr/bin/env node
-// Give this signal-forwarding wrapper a meaningful process name. Under Node 24+
+// Give the CLI process a meaningful process name. Under Node 24+
 // V8 names the main thread "MainThread", so without this every pty process shows
 // up as `MainThread` in ps/top/htop/btm. `process.title` is the only thing that
 // overrides /proc/<pid>/comm, and only when set from within the running process
 // after V8 init. Linux caps comm at 15 chars (TASK_COMM_LEN), so keep it short.
 try { process.title = 'pty'; } catch {}
 
 import { existsSync } from 'node:fs';
-import { spawn } from 'node:child_process';
-import { fileURLToPath } from 'node:url';
+import { fileURLToPath, pathToFileURL } from 'node:url';
 import { dirname, join } from 'node:path';
-import { constants as osConstants } from 'node:os';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 const cli = join(__dirname, '..', 'dist', 'cli.js');
@@ -20,38 +18,7 @@ if (!existsSync(cli)) {
   process.exit(1);
 }
 
-// `spawn` (not `spawnSync`) so the wrapper stays event-loop-live and can
-// react to signals it receives. Without this, signals delivered to the
-// wrapper (e.g. systemd's SIGTERM under KillMode=process) terminated the
-// wrapper without ever reaching the child supervisor, leaving an orphaned
-// supervisor that kept holding `supervisor.lock` and made every subsequent
-// service restart fail with "another supervisor is already running".
-const child = spawn(process.execPath, [cli, ...process.argv.slice(2)], {
-  stdio: 'inherit',
-  env: process.env,
-});
-
-const FORWARDED_SIGNALS = ['SIGTERM', 'SIGINT', 'SIGHUP', 'SIGQUIT', 'SIGUSR1', 'SIGUSR2'];
-for (const sig of FORWARDED_SIGNALS) {
-  process.on(sig, () => {
-    // Forward and let the child decide when to exit. The wrapper's own
-    // exit is driven by the child's 'exit' event so the child has time
-    // to flush before we propagate its status.
-    try { child.kill(sig); } catch {}
-  });
-}
-
-child.on('exit', (code, signal) => {
-  if (signal) {
-    // Mirror shell convention (128 + signum) so callers can distinguish
-    // signal death from a nonzero exit code.
-    const signum = osConstants.signals[signal] ?? 0;
-    process.exit(128 + signum);
-  }
-  process.exit(code ?? 1);
-});
-
-child.on('error', (err) => {
-  console.error(`pty: failed to spawn cli: ${err.message}`);
-  process.exit(1);
-});
+// Load the CLI in this process. Besides avoiding an otherwise redundant Node
+// child and signal-forwarding layer, this preserves every descriptor inherited
+// by the launcher, including caller-owned machine-stream descriptors above 2.
+await import(pathToFileURL(cli).href);

src/cli.ts

@@ -2796,8 +2796,8 @@ function printLaunchdPlist(interval: number): void {
   const escape = (s: string) =>
     s.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;");
 
-  // We point ProgramArguments at node + the resolved CLI script so the
-  // plist doesn't depend on the `pty` shim staying on PATH at launchd's
+  // We point ProgramArguments at node + the invoked launcher or CLI script so
+  // the plist doesn't depend on the `pty` shim staying on PATH at launchd's
   // (minimal) shell. EnvironmentVariables still carries PATH so the
   // spawned children (and any `which` inside pty itself) find the user's
   // tools. PTY_ROOT (canonical) pins the target registry.

tests/attach-stream.test.ts

@@ -19,6 +19,7 @@ import {
 
 const __dirname = path.dirname(fileURLToPath(import.meta.url));
 const cliPath = path.join(__dirname, "..", "dist", "cli.js");
+const binPath = path.join(__dirname, "..", "bin", "pty");
 const clientUrl = pathToFileURL(path.join(__dirname, "..", "dist", "client.js")).href;
 const nodeBin = process.execPath;
 const testRoot = fs.mkdtempSync(path.join(os.tmpdir(), "pty-attach-stream-"));
@@ -122,6 +123,63 @@ describe("pty attach --attach-stream-fd-v1", () => {
     expect(result.stdout).toBe("");
   });
 
+  it("streams framed events through fd 3 from the shipped bin launcher", async () => {
+    const root = fs.mkdtempSync(path.join(testRoot, "launcher-"));
+    const name = `launcher-${process.pid}`;
+    const launcherEnv: NodeJS.ProcessEnv = {
+      ...process.env,
+      PTY_ROOT: root,
+      PTY_ROOT_LEGACY_SILENT: "1",
+    };
+    delete launcherEnv.PTY_SESSION;
+    delete launcherEnv.PTY_SERVER_CONFIG;
+    const created = spawnSync(
+      nodeBin,
+      [cliPath, "run", "-d", "--id", name, "--", "sh", "-c", "printf LAUNCHER_READY; read value"],
+      { env: launcherEnv, encoding: "utf8" },
+    );
+    expect(created.status, created.stderr).toBe(0);
+    const child = spawn(
+      nodeBin,
+      [binPath, "attach", "--attach-stream-fd-v1", "3", name],
+      {
+        env: launcherEnv,
+        stdio: ["pipe", "pipe", "pipe", "pipe"],
+      },
+    );
+    const stdout = collect(child.stdout);
+    const stderr = collect(child.stderr);
+    const streamChunks: Buffer[] = [];
+    const reader = new PacketReader();
+    let sentInput = false;
+    (child.stdio[3] as NodeJS.ReadableStream).on("data", (chunk) => {
+      const data = Buffer.from(chunk);
+      streamChunks.push(data);
+      for (const packet of reader.feed(data)) {
+        if (packet.type === MessageType.SCREEN && !sentInput) {
+          sentInput = true;
+          child.stdin.write("done\n");
+        }
+      }
+    });
+    const status = await new Promise<number | null>((resolve, reject) => {
+      const timer = setTimeout(() => reject(new Error("bin launcher attach timed out")), 10_000);
+      child.once("exit", (code) => {
+        clearTimeout(timer);
+        resolve(code);
+      });
+    });
+
+    expect(status).toBe(0);
+    expect(await stdout).toEqual(Buffer.alloc(0));
+    expect(await stderr).toEqual(Buffer.alloc(0));
+    const packets = new PacketReader().feed(Buffer.concat(streamChunks));
+    expect(packets[0].type).toBe(MessageType.GEOMETRY);
+    expect(packets[1].type).toBe(MessageType.SCREEN);
+    expect(packets[1].payload.toString()).toContain("LAUNCHER_READY");
+    expect(packets.at(-1)?.type).toBe(MessageType.EXIT);
+  }, 15_000);
+
   it("reframes fragmented and coalesced daemon packets in order without stdout output", async () => {
     const geometry = encodeGeometry(31, 97);
     const screen = encodeScreen("\x1b[31mred\x1b[0m");

tests/gc.test.ts

@@ -9,6 +9,7 @@ import { spawn, spawnSync } from "node:child_process";
 const __dirname = path.dirname(fileURLToPath(import.meta.url));
 const nodeBin = process.execPath;
 const cliPath = path.join(__dirname, "..", "dist", "cli.js");
+const binPath = path.join(__dirname, "..", "bin", "pty");
 const serverModule = path.join(__dirname, "..", "dist", "server.js");
 
 const testRoot = fs.mkdtempSync(path.join(os.tmpdir(), "pty-gc-"));
@@ -299,6 +300,17 @@ describe("pty gc", () => {
     expect(result.stdout).toContain("<key>PTY_ROOT</key>");
   });
 
+  it("--print-launchd-plist preserves the invoked bin launcher", () => {
+    const dir = makeSessionDir();
+    const result = spawnSync(nodeBin, [binPath, "gc", "--print-launchd-plist"], {
+      env: { ...process.env, PTY_SESSION_DIR: dir },
+      encoding: "utf-8",
+      timeout: 10000,
+    });
+    expect(result.status).toBe(0);
+    expect(result.stdout).toContain(`<string>${binPath}</string>`);
+  });
+
   it("--print-launchd-plist --interval=N sets the interval", () => {
     const dir = makeSessionDir();
     const result = runCli(dir, "gc", "--print-launchd-plist", "--interval=15");

tests/wrapper-signal-forwarding.test.ts

@@ -1,16 +1,13 @@
-// Verifies that bin/pty forwards SIGTERM/SIGINT to the inner cli.js child.
-// systemd's `KillMode=process` only signals the leader (the bin/pty shell
-// shim) and lets children become orphans unless the leader propagates the
-// signal. Without forwarding, the inner cli.js survives a unit `stop` and
-// the next start fails because the orphan still holds whatever resource
-// it owned (file watchers, sockets, etc.).
+// Verifies that bin/pty runs the CLI in the launcher's process. Keeping a
+// single process makes inherited descriptors and signal ownership identical
+// to a direct dist/cli.js invocation.
 
 import { describe, it, expect, afterEach } from "vitest";
 import * as fs from "node:fs";
 import * as os from "node:os";
 import * as path from "node:path";
 import { fileURLToPath } from "node:url";
-import { spawn } from "node:child_process";
+import { execFileSync, spawn } from "node:child_process";
 
 const __dirname = path.dirname(fileURLToPath(import.meta.url));
 const wrapperPath = path.join(__dirname, "..", "bin", "pty");
@@ -49,18 +46,16 @@ function isAlive(pid: number): boolean {
   try { process.kill(pid, 0); return true; } catch { return false; }
 }
 
-describe("bin/pty signal forwarding", () => {
-  it("propagates SIGTERM to the inner cli.js (events --all is long-lived)", async () => {
+describe("bin/pty process lifecycle", () => {
+  it("runs a long-lived command as one process that exits on SIGTERM", async () => {
     const sessionDir = makeSessionDir();
+    const socketPath = path.join(sessionDir, "remote.sock");
 
-    // `events --all` is a long-lived command that registers a SIGINT
-    // handler and runs an EventFollower until the process is signalled.
-    // Same shape as the supervisor used to be: long-running, owns file
-    // watchers, must exit cleanly when the wrapper relays a signal.
-    const wrapper = spawn(nodeBin, [wrapperPath, "events", "--all"], {
+    const wrapper = spawn(nodeBin, [wrapperPath, "remote-serve", "--socket", socketPath], {
       env: { ...process.env, PTY_SESSION_DIR: sessionDir },
       stdio: ["ignore", "pipe", "pipe"],
     });
+    trackedPids.push(wrapper.pid!);
 
     let stdout = "";
     let stderr = "";
@@ -71,36 +66,27 @@ describe("bin/pty signal forwarding", () => {
     let exitSignal: NodeJS.Signals | null = null;
     wrapper.on("exit", (c, s) => { exitCode = c; exitSignal = s; });
 
-    // Wait long enough for the wrapper to fork the inner node cli.js.
-    // No specific marker; sleep briefly then look at the process tree.
-    await new Promise((r) => setTimeout(r, 800));
+    const started = await waitFor(
+      () => stdout.includes(`pty remote-serve listening on ${socketPath}`),
+      5000,
+    );
+    expect(started, `bin/pty did not become ready; stdout=${stdout} stderr=${stderr}`).toBe(true);
 
-    // Find the inner cli.js by walking the wrapper's child processes via
-    // /proc on Linux, or via `pgrep -P` everywhere. We use ps -o pid -p
-    // <wrapper-pid> first then list children with a tree walk fallback.
-    const psResult = (() => {
+    const childPids = (() => {
       try {
-        const { execFileSync } = require("node:child_process") as typeof import("node:child_process");
         return execFileSync("pgrep", ["-P", String(wrapper.pid)], { encoding: "utf-8" }).trim();
       } catch {
         return "";
       }
     })();
-    expect(psResult, `pgrep failed to find a child of pid ${wrapper.pid}; stdout=${stdout} stderr=${stderr}`).not.toBe("");
+    expect(childPids, `bin/pty unexpectedly spawned a child; stdout=${stdout} stderr=${stderr}`).toBe("");
 
-    const innerPid = parseInt(psResult.split("\n")[0]!.trim(), 10);
-    trackedPids.push(innerPid);
-    expect(innerPid).toBeGreaterThan(0);
-    expect(innerPid).not.toBe(wrapper.pid);
-    expect(isAlive(innerPid)).toBe(true);
-
-    // The actual test: SIGTERM the wrapper, expect the inner cli.js to also die.
     wrapper.kill("SIGTERM");
 
     const wrapperExited = await waitFor(() => exitCode !== null || exitSignal !== null, 5000);
     expect(wrapperExited, "wrapper did not exit after SIGTERM").toBe(true);
 
-    const innerDied = await waitFor(() => !isAlive(innerPid), 5000);
-    expect(innerDied, `inner cli.js (pid ${innerPid}) survived wrapper SIGTERM — signal not forwarded`).toBe(true);
+    expect(exitCode).toBe(0);
+    expect(exitSignal).toBeNull();
   }, 20000);
 });