diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..6235323 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,15 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [0.1.3] - 2026-02-21 + +### Fixed + +- Graceful shutdown: close WebSocket clients before killing PTYs to prevent node-pty native crashes on teardown. ([#20](https://github.com/holiber/jabterm/pull/20)) +- Make `close()` idempotent under concurrent callers by returning a single shared close promise. ([#20](https://github.com/holiber/jabterm/pull/20)) +- CLI: allow `--port 0` to correctly request an ephemeral port. ([#20](https://github.com/holiber/jabterm/pull/20)) + diff --git a/bin/jabterm-server.mjs b/bin/jabterm-server.mjs old mode 100644 new mode 100755 index a741841..9052e19 --- a/bin/jabterm-server.mjs +++ b/bin/jabterm-server.mjs @@ -10,53 +10,10 @@ */ import { createTerminalServer } from "../dist/server/index.js"; +import { parseCliArgs, resolveCliPort } from "../dist/server/cli.js"; -function parseArgs(argv) { - const args = {}; - for (let i = 0; i < argv.length; i++) { - const arg = argv[i]; - if (arg === "--port" && argv[i + 1]) { - args.port = parseInt(argv[++i], 10); - } else if (arg.startsWith("--port=")) { - args.port = parseInt(arg.slice(7), 10); - } else if (arg === "--shell" && argv[i + 1]) { - args.shell = argv[++i]; - } else if (arg.startsWith("--shell=")) { - args.shell = arg.slice(8); - } else if (arg === "--cwd" && argv[i + 1]) { - args.cwd = argv[++i]; - } else if (arg.startsWith("--cwd=")) { - args.cwd = arg.slice(6); - } else if (arg === "--host" && argv[i + 1]) { - args.host = argv[++i]; - } else if (arg.startsWith("--host=")) { - args.host = arg.slice(7); - } else if (arg === "--strict-port") { - args.strictPort = true; - } else if (arg === "--help" || arg === "-h") { - console.log(` - jabterm-server - JabTerm WebSocket terminal server - - Usage: - jabterm-server [options] - - Options: - --port Port to listen on (default: 3223, or JABTERM_PORT env) - --host Host to bind to (default: 127.0.0.1) - --shell Shell to spawn (default: uses $SHELL when available; otherwise bash/sh on Linux and zsh/bash/sh on macOS; powershell.exe on Windows) - --cwd Working directory for new terminals (default: $HOME) - --strict-port Fail if port is already in use - -h, --help Show this help -`); - process.exit(0); - } - } - return args; -} - -const args = parseArgs(process.argv.slice(2)); - -const port = args.port || parseInt(process.env.JABTERM_PORT || "3223", 10); +const args = parseCliArgs(process.argv.slice(2)); +const port = resolveCliPort(args, process.env); const server = await createTerminalServer({ port, diff --git a/package.json b/package.json index 85a9ff5..3c6fbe8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "jabterm", - "version": "0.1.2", + "version": "0.1.3", "description": "JabTerm - Just Another Browser Terminal. Drop-in React component + Node.js server.", "type": "module", "exports": { diff --git a/src/server/cli.ts b/src/server/cli.ts new file mode 100644 index 0000000..4e2b06f --- /dev/null +++ b/src/server/cli.ts @@ -0,0 +1,58 @@ +export interface JabtermCliArgs { + port?: number; + shell?: string; + cwd?: string; + host?: string; + strictPort?: boolean; +} + +export function parseCliArgs(argv: string[]): JabtermCliArgs { + const args: JabtermCliArgs = {}; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === "--port" && argv[i + 1]) { + args.port = parseInt(argv[++i], 10); + } else if (arg.startsWith("--port=")) { + args.port = parseInt(arg.slice(7), 10); + } else if (arg === "--shell" && argv[i + 1]) { + args.shell = argv[++i]; + } else if (arg.startsWith("--shell=")) { + args.shell = arg.slice(8); + } else if (arg === "--cwd" && argv[i + 1]) { + args.cwd = argv[++i]; + } else if (arg.startsWith("--cwd=")) { + args.cwd = arg.slice(6); + } else if (arg === "--host" && argv[i + 1]) { + args.host = argv[++i]; + } else if (arg.startsWith("--host=")) { + args.host = arg.slice(7); + } else if (arg === "--strict-port") { + args.strictPort = true; + } else if (arg === "--help" || arg === "-h") { + console.log(` + jabterm-server - JabTerm WebSocket terminal server + + Usage: + jabterm-server [options] + + Options: + --port Port to listen on (default: 3223, or JABTERM_PORT env) + --host Host to bind to (default: 127.0.0.1) + --shell Shell to spawn (default: uses $SHELL when available; otherwise bash/sh on Linux and zsh/bash/sh on macOS; powershell.exe on Windows) + --cwd Working directory for new terminals (default: $HOME) + --strict-port Fail if port is already in use + -h, --help Show this help +`); + process.exit(0); + } + } + return args; +} + +export function resolveCliPort( + args: JabtermCliArgs, + env: NodeJS.ProcessEnv = process.env, +): number { + return args.port ?? parseInt(env.JABTERM_PORT || "3223", 10); +} + diff --git a/src/server/jabtermServer.ts b/src/server/jabtermServer.ts index 74297c8..2b23b23 100644 --- a/src/server/jabtermServer.ts +++ b/src/server/jabtermServer.ts @@ -67,8 +67,8 @@ export interface JabtermServerOptions { * If `Origin` is absent (e.g. node `ws` client), the connection is allowed. */ allowedOrigins?: - | string[] - | ((origin: string | undefined, req: IncomingMessage) => boolean); + | string[] + | ((origin: string | undefined, req: IncomingMessage) => boolean); /** Optional structured logger. */ logger?: JabtermLogger; /** @@ -129,11 +129,11 @@ function replyHttpError(socket: Duplex, status: number, reason: string) { try { socket.write( `HTTP/1.1 ${status} ${reason}\r\n` + - "Connection: close\r\n" + - "Content-Type: text/plain; charset=utf-8\r\n" + - `Content-Length: ${Buffer.byteLength(reason)}\r\n` + - "\r\n" + - reason, + "Connection: close\r\n" + + "Content-Type: text/plain; charset=utf-8\r\n" + + `Content-Length: ${Buffer.byteLength(reason)}\r\n` + + "\r\n" + + reason, ); } catch { /* ignore */ @@ -309,7 +309,7 @@ export function createJabtermServer(opts: JabtermServerOptions = {}): JabtermSer ptyExited: false, wsClosed: false, helloVersion: undefined, - onWsMessage: () => {}, + onWsMessage: () => { }, }; sessions.add(session); @@ -483,12 +483,13 @@ export function createJabtermServer(opts: JabtermServerOptions = {}): JabtermSer return { address: addr.address, family: addr.family, port: addr.port }; } - async function close(): Promise { - if (closing) return closePromise ?? Promise.resolve(); + function close(): Promise { + if (closePromise) return closePromise; closing = true; logger.info?.("shutting_down"); - const killAndCloseClients = () => { + const killAndCloseClients = async () => { + // Phase 1: Close all WS connections and detach message handlers for (const session of sessions) { try { session.wsClosed = true; @@ -515,6 +516,15 @@ export function createJabtermServer(opts: JabtermServerOptions = {}): JabtermSer } catch { /* ignore */ } + } + + // Wait for WS close frames to be processed and onData handlers to detach. + // Without this, killing PTY while onData is still wired to a closing WS + // triggers an uncatchable native C++ exception in node-pty (SIGABRT). + await new Promise((r) => setTimeout(r, 100)); + + // Phase 2: Kill PTY processes (safe — WS clients are disconnected) + for (const session of sessions) { try { if (!session.ptyExited) session.pty.kill(); } catch { @@ -532,9 +542,8 @@ export function createJabtermServer(opts: JabtermServerOptions = {}): JabtermSer ptys.clear(); }; - killAndCloseClients(); - closePromise = (async () => { + await killAndCloseClients(); await new Promise((resolve) => { let done = false; const finish = () => { diff --git a/tests/unit/server/cli.test.ts b/tests/unit/server/cli.test.ts new file mode 100644 index 0000000..74fbffd --- /dev/null +++ b/tests/unit/server/cli.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; + +import { parseCliArgs, resolveCliPort } from "../../../src/server/cli.js"; + +describe("server cli helpers", () => { + it("preserves --port 0 (space form)", () => { + const args = parseCliArgs(["--port", "0"]); + expect(args.port).toBe(0); + expect(resolveCliPort(args, { JABTERM_PORT: "3223" })).toBe(0); + }); + + it("preserves --port=0 (equals form)", () => { + const args = parseCliArgs(["--port=0"]); + expect(args.port).toBe(0); + expect(resolveCliPort(args, { JABTERM_PORT: "3223" })).toBe(0); + }); +}); + diff --git a/tests/unit/server/close-idempotent.test.ts b/tests/unit/server/close-idempotent.test.ts new file mode 100644 index 0000000..b39b612 --- /dev/null +++ b/tests/unit/server/close-idempotent.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it, vi } from "vitest"; + +import { WebSocket } from "ws"; + +type ExitHandler = (e: { exitCode: number; signal?: number }) => void; + +function deferred() { + let resolve!: (v: T) => void; + let reject!: (e: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +vi.mock("node-pty", () => { + return { + spawn: vi.fn(() => { + let onExit: ExitHandler | null = null; + let killed = false; + return { + onData() { + // ignore + }, + onExit(cb: ExitHandler) { + onExit = cb; + }, + write() { + // ignore + }, + resize() { + // ignore + }, + kill() { + if (killed) return; + killed = true; + onExit?.({ exitCode: 0 }); + }, + }; + }), + }; +}); + +describe("createJabtermServer().close()", () => { + it("is idempotent and returns the same promise for concurrent callers", async () => { + const { createJabtermServer } = await import("../../../src/server/jabtermServer.js"); + + const logger = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }; + const server = createJabtermServer({ host: "127.0.0.1", port: 0, logger }); + const addr = await server.listen(); + + const wsClosed = deferred(); + const ws = new WebSocket(`ws://${addr.address}:${addr.port}/`); + ws.once("open", () => { + // Trigger an active session to exist. + ws.send(JSON.stringify({ type: "hello", version: 1 })); + }); + ws.once("close", () => wsClosed.resolve()); + ws.once("error", (e) => wsClosed.reject(e)); + + // Ensure the connection attempt has started. + await new Promise((r) => setTimeout(r, 25)); + + const p1 = server.close(); + const p2 = server.close(); + expect(p2).toBe(p1); + + await Promise.all([p1, p2, wsClosed.promise]); + expect(() => server.address()).toThrow(/not listening/i); + + // Already-closed calls should keep working. + await expect(server.close()).resolves.toBeUndefined(); + }); +}); +