Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions BRANCH_DETAILS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Windows terminal startup

**Worktree branch:** `fix/windows-terminal-startup`

Windows worktree: `E:/Projects/t3code.worktrees/windows-terminal-startup`.

node-pty 1.2 initializes ConPTY asynchronously and initially reports PID zero. The node-pty adapter waits for its `ready_datapipe` event before returning the process to the terminal manager. This keeps startup snapshots within the existing positive-PID contract without waiting for shell output or polling. An exit before readiness becomes a typed spawn failure. Interruption removes listeners and terminates the pending ConPTY through its agent without waiting for output. Readiness without a positive PID requests termination and fails the spawn. Unix startup keeps its synchronous path.

The readiness event and immediate cancellation through `_agent.kill()` are node-pty internals outside its public TypeScript interface. Keep these dependencies isolated in the adapter and recheck them when upgrading node-pty. The fix applies to all clients opening terminals on a Windows environment, including remote connections, without changing client or wire contracts.

Focused regression coverage:

```sh
vp test run apps/server/src/terminal/NodePtyAdapter.test.ts apps/server/src/terminal/Manager.test.ts
```

Tests cover delayed PID assignment without output, output and exit delivery after startup, early process exit, output-independent interruption cleanup, invalid-PID readiness failure, and existing Windows, Linux, and macOS adapter behavior.

Windows Browser-panel verification covers command output, multiple terminals, closing a terminal, and reattaching after a page reload. Run the backend without Node's `--watch` mode for this check: node-pty misreads watcher IPC as a process-list response and crashes when closing a terminal. That separate dev-mode issue remains outstanding.
130 changes: 122 additions & 8 deletions apps/server/src/terminal/NodePtyAdapter.test.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,45 @@
import * as NodeEvents from "node:events";

import * as NodeServices from "@effect/platform-node/NodeServices";
import { assert, it } from "@effect/vitest";
import { HostProcessArchitecture, HostProcessPlatform } from "@t3tools/shared/hostProcess";
import * as Cause from "effect/Cause";
import * as Deferred from "effect/Deferred";
import * as Effect from "effect/Effect";
import * as Exit from "effect/Exit";
import * as Fiber from "effect/Fiber";
import * as Layer from "effect/Layer";
import { vi } from "vite-plus/test";

import * as NodePtyAdapter from "./NodePtyAdapter.ts";
import * as PtyAdapter from "./PtyAdapter.ts";

const spawn = vi.fn(() => ({
pid: 42,
write: vi.fn(),
resize: vi.fn(),
kill: vi.fn(),
onData: vi.fn(() => ({ dispose: vi.fn() })),
onExit: vi.fn(() => ({ dispose: vi.fn() })),
}));
const makeNativeProcess = () => {
const events = new NodeEvents.EventEmitter();
const agent = { kill: vi.fn() };
return {
_agent: agent,
pid: 42,
events,
write: vi.fn(),
resize: vi.fn(),
kill: vi.fn(),
on: vi.fn((event: string, listener: () => void) => events.on(event, listener)),
removeListener: vi.fn((event: string, listener: () => void) =>
events.removeListener(event, listener),
),
onData: vi.fn((listener: (data: string) => void) => {
events.on("data", listener);
return { dispose: vi.fn(() => events.removeListener("data", listener)) };
}),
onExit: vi.fn((listener: (event: { exitCode: number; signal?: number }) => void) => {
events.on("exit", listener);
return { dispose: vi.fn(() => events.removeListener("exit", listener)) };
}),
};
};

const spawn = vi.fn(makeNativeProcess);

const fakeNodePty = { spawn } as unknown as typeof import("node-pty");

Expand All @@ -35,6 +57,98 @@ const makeTestLayer = (platform: NodeJS.Platform = "win32") =>

const testLayer = makeTestLayer();

const startPendingSpawn = Effect.gen(function* () {
const listening = yield* Deferred.make<void>();
const nativeProcess = makeNativeProcess();
nativeProcess.pid = 0;
nativeProcess.on.mockImplementation((event, listener) => {
nativeProcess.events.on(event, listener);
Deferred.doneUnsafe(listening, Effect.void);
return nativeProcess.events;
});
spawn.mockReturnValueOnce(nativeProcess);
const adapter = yield* PtyAdapter.PtyAdapter;
const fiber = yield* adapter
.spawn({ shell: "powershell.exe", cwd: ".", cols: 80, rows: 24, env: {} })
.pipe(Effect.forkChild);
yield* Deferred.await(listening);
return { nativeProcess, fiber };
});

it.effect("waits for the Windows PID without requiring shell output", () =>
Effect.gen(function* () {
const { nativeProcess, fiber } = yield* startPendingSpawn;
assert.isUndefined(fiber.pollUnsafe());

nativeProcess.pid = 123;
nativeProcess.events.emit("ready_datapipe");
const process = yield* Fiber.join(fiber);
assert.equal(process.pid, 123);
assert.equal(nativeProcess.events.listenerCount("ready_datapipe"), 0);
assert.equal(nativeProcess.events.listenerCount("exit"), 0);

const output: string[] = [];
const exits: PtyAdapter.PtyExitEvent[] = [];
const stopData = process.onData((data) => output.push(data));
const stopExit = process.onExit((event) => exits.push(event));
nativeProcess.events.emit("data", "first prompt");
nativeProcess.events.emit("exit", { exitCode: 0 });
assert.deepEqual(output, ["first prompt"]);
assert.deepEqual(exits, [{ exitCode: 0, signal: null }]);
stopData();
stopExit();
}).pipe(Effect.provide(testLayer)),
);

it.effect("reports Windows exit before readiness as a spawn failure", () =>
Effect.gen(function* () {
const { nativeProcess, fiber } = yield* startPendingSpawn;
nativeProcess.events.emit("exit", { exitCode: 2 });
const exit = yield* Fiber.await(fiber);
assert.isTrue(Exit.isFailure(exit));
if (Exit.isFailure(exit)) {
const error = Cause.squash(exit.cause);
assert.instanceOf(error, PtyAdapter.PtySpawnError);
assert.match(String(error.cause), /exit code 2/);
}
assert.equal(nativeProcess.events.listenerCount("ready_datapipe"), 0);
assert.equal(nativeProcess.events.listenerCount("exit"), 0);
}).pipe(Effect.provide(testLayer)),
);

it.effect("cleans up a Windows spawn interrupted before readiness", () =>
Effect.gen(function* () {
const { nativeProcess, fiber } = yield* startPendingSpawn;
nativeProcess.kill.mockImplementation(() => {
nativeProcess.events.once("data", () => nativeProcess._agent.kill());
});
yield* Fiber.interrupt(fiber);
assert.equal(nativeProcess._agent.kill.mock.calls.length, 1);
assert.equal(nativeProcess.kill.mock.calls.length, 0);
assert.equal(nativeProcess.events.listenerCount("ready_datapipe"), 0);
assert.equal(nativeProcess.events.listenerCount("exit"), 0);
nativeProcess.events.emit("data", "late output");
assert.equal(nativeProcess._agent.kill.mock.calls.length, 1);
}).pipe(Effect.provide(testLayer)),
);

it.effect("requests termination when Windows readiness has no process ID", () =>
Effect.gen(function* () {
const { nativeProcess, fiber } = yield* startPendingSpawn;
nativeProcess.events.emit("ready_datapipe");
const exit = yield* Fiber.await(fiber);
assert.isTrue(Exit.isFailure(exit));
if (Exit.isFailure(exit)) {
const error = Cause.squash(exit.cause);
assert.instanceOf(error, PtyAdapter.PtySpawnError);
assert.match(String(error.cause), /process ID/);
}
assert.equal(nativeProcess.kill.mock.calls.length, 1);
assert.equal(nativeProcess.events.listenerCount("ready_datapipe"), 0);
assert.equal(nativeProcess.events.listenerCount("exit"), 0);
}).pipe(Effect.provide(testLayer)),
);

for (const platform of ["win32", "linux", "darwin"] as const) {
it.effect(`terminates through node-pty using ${platform} semantics`, () =>
Effect.gen(function* () {
Expand Down
53 changes: 53 additions & 0 deletions apps/server/src/terminal/NodePtyAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,56 @@ class NodePtyProcess implements PtyAdapter.PtyProcess {
}
}

// node-pty's Windows PID is assigned asynchronously, before this legacy socket
// event fires. Waiting for output instead would hang shells with a silent prompt.
type WindowsPty = import("node-pty").IPty & {
_agent: { kill(): void };
on(event: "ready_datapipe", listener: () => void): void;
removeListener(event: "ready_datapipe", listener: () => void): void;
};

const awaitWindowsPtyReady = (process: WindowsPty, shell: string) =>
Effect.callback<void, PtyAdapter.PtySpawnError>((resume) => {
const onReady = () => {
cleanup();
if (process.pid <= 0) process.kill();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
resume(
process.pid > 0
? Effect.void
: Effect.fail(
new PtyAdapter.PtySpawnError({
adapter: "node-pty",
shell,
cause: new Error("ConPTY connected without a process ID."),
}),
),
);
};
const exitListener = process.onExit((event) => {
cleanup();
resume(
Effect.fail(
new PtyAdapter.PtySpawnError({
adapter: "node-pty",
shell,
cause: new Error(`ConPTY exited before startup (exit code ${event.exitCode}).`),
}),
),
);
});
const cleanup = () => {
process.removeListener("ready_datapipe", onReady);
exitListener.dispose();
};
process.on("ready_datapipe", onReady);
return Effect.sync(() => {
cleanup();
// Public kill waits for the first output byte. The agent can cancel the
// pending connection before it launches a child, even without output.
process._agent.kill();
});
});

export const make = Effect.fn("NodePtyAdapter.make")(function* () {
const loadNodePtyModule = yield* NodePtyModuleLoaderRef;
const fs = yield* FileSystem.FileSystem;
Expand Down Expand Up @@ -181,6 +231,9 @@ export const make = Effect.fn("NodePtyAdapter.make")(function* () {
cause,
}),
});
if (platform === "win32" && ptyProcess.pid === 0) {
yield* awaitWindowsPtyReady(ptyProcess as WindowsPty, input.shell);
}
return new NodePtyProcess(ptyProcess, platform);
}),
});
Expand Down
Loading