Keep nmshd alive when a resize races a shell's exit (#199)

CHANGELOG.md

@@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - **Session limit:** at most 16 live sessions per service (`NMSH_MAX_SESSIONS`). When the limit is reached, a new window falls back to an in-process shell with a notice. Detached sessions are never ended to make room.
 
 ### Fixed
+- The session service no longer crashes when a window resize races a shell's exit. node-pty could throw `EBADF` for a PTY that had just closed, which inside nmshd would have ended every live session. Other resize failures are now reported to that window instead of ending the service.
 - A launch notice (for example, sessions archived while no window was attached) is no longer erased when the window reattaches to a live session.
 
 ### Removed

src/session/SessionService.ts

@@ -282,7 +282,7 @@ export class SessionService {
             break;
           case 'input': owned?.evidence.onInput(); owned?.shell.write(message.data); break;
           case 'resize':
-            if (owned) { owned.resizes += 1; owned.shell.resize(message.columns, message.rows); }
+            if (owned) { owned.resizes += 1; this.resize(owned, message.columns, message.rows, send); }
             break;
           case 'ack': owned?.backlog.ack(message.seq, message.journalId); break;
           case 'kill': {
@@ -325,13 +325,27 @@ export class SessionService {
    * delivers SIGWINCH and the final one lands on the new frontend's size.
    */
   private redraw(session: ManagedSession, columns: number, rows: number): void {
-    session.shell.resize(columns, rows > 2 ? rows - 1 : rows + 1);
+    this.resize(session, columns, rows > 2 ? rows - 1 : rows + 1, session.controller);
     const generation = session.resizes;
     setTimeout(() => {
-      if (this.sessions.has(session.record.id) && session.resizes === generation) session.shell.resize(columns, rows);
+      if (this.sessions.has(session.record.id) && session.resizes === generation) this.resize(session, columns, rows, session.controller);
     }, REDRAW_NUDGE_MS);
   }
 
+  /**
+   * Resize one session's PTY without letting a failure escape into the
+   * service: nmshd owns every live session, so one session's PTY error must
+   * never end the others. ShellSession already ignores the benign teardown
+   * race; anything else is reported to the client that caused it.
+   */
+  private resize(session: ManagedSession, columns: number, rows: number, send?: Send): void {
+    try {
+      session.shell.resize(columns, rows);
+    } catch (error) {
+      send?.({type: 'error', code: 'resize', message: `could not resize the session: ${error instanceof Error ? error.message : String(error)}`});
+    }
+  }
+
   private create(cwd: string, env: Record<string, string>, columns: number, rows: number, send: Send): ManagedSession {
     // The shell gets the launching frontend's environment and cwd, never the
     // service's own startup state. The env is opaque: it is not stored or logged.

src/shell/ShellSession.ts

@@ -13,11 +13,18 @@ interface SessionEvents {
   exit: [{ exitCode: number; signal?: number }];
 }
 
+/** node-pty's error for an ioctl on a PTY whose descriptor is already closed. */
+export function isClosedPtyError(error: unknown): boolean {
+  return error instanceof Error && /\bEBADF\b/u.test(error.message);
+}
+
 export class ShellSession extends EventEmitter<SessionEvents> {
   private readonly pty: IPty;
   private readonly protocol: ShellProtocolDecoder;
   private zdotdir: string;
   private ready = false;
+  /** Set once the shell has exited or its PTY is closed; resizes after that are no-ops. */
+  private exited = false;
 
   constructor(cwd: string, columns: number, rows: number, home = process.env.HOME || '', env: NodeJS.ProcessEnv = process.env) {
     super();
@@ -115,6 +122,7 @@ add-zsh-hook preexec nmsh_preexec
 
     this.pty.onData(data => this.receive(data));
     this.pty.onExit(event => {
+      this.exited = true;
       this.cleanup();
       this.emit('exit', event);
     });
@@ -156,8 +164,21 @@ add-zsh-hook preexec nmsh_preexec
     this.pty.write('\u0004');
   }
 
+  /**
+   * Resize the PTY. A resize can legitimately race the shell's teardown: a
+   * frontend may send one just before it hears of the exit, and node-pty closes
+   * the PTY's descriptor before it reports the exit. Once the shell is gone
+   * there is nothing to resize, so that case is ignored; any other failure
+   * still throws.
+   */
   resize(columns: number, rows: number): void {
-    this.pty.resize(Math.max(2, columns), Math.max(2, rows));
+    if (this.exited) return;
+    try {
+      this.pty.resize(Math.max(2, columns), Math.max(2, rows));
+    } catch (error) {
+      if (!isClosedPtyError(error)) throw error;
+      this.exited = true;
+    }
   }
 
   kill(): void {

tests/ptyResizeRace.test.ts

@@ -0,0 +1,179 @@
+import test from 'node:test';
+import assert from 'node:assert/strict';
+import {chmodSync, existsSync, mkdtempSync, realpathSync, rmSync} from 'node:fs';
+import {connect, type Socket} from 'node:net';
+import {tmpdir} from 'node:os';
+import {join} from 'node:path';
+import {SessionService} from '../src/session/SessionService.js';
+import {FrameDecoder, PROTOCOL_VERSION, encodeMessage, type ServerMessage} from '../src/session/SessionProtocol.js';
+import {ShellSession, isClosedPtyError} from '../src/shell/ShellSession.js';
+import {until} from './helpers/liveFrontend.js';
+
+/**
+ * #199: node-pty closes the PTY descriptor before it reports the shell's exit,
+ * and a frontend can always send a resize just before it hears of the exit.
+ * A resize on a closed PTY threw `ioctl(2) failed, EBADF` synchronously, which
+ * inside nmshd's socket handler would end the service and every live session.
+ */
+
+function scratch(prefix: string): string {
+  const dir = realpathSync(mkdtempSync(join(tmpdir(), prefix)));
+  chmodSync(dir, 0o700);
+  return dir;
+}
+
+/**
+ * Close the service and wait for its shells to exit: a shell that ends after
+ * close still writes its spool (the exit record), which would otherwise
+ * recreate the runtime directory after the test removed it.
+ */
+async function closeAndDrain(service: SessionService): Promise<void> {
+  const pids = service.registry.map(session => session.pid);
+  await service.close();
+  await until(() => pids.every(pid => { try { process.kill(pid, 0); return false; } catch { return true; } }), 10000, 'shells exited');
+  await new Promise(resolve => setTimeout(resolve, 50));
+}
+
+const prompt = (shell: ShellSession) => new Promise<void>(resolve => shell.once('prompt', () => resolve()));
+const exit = (shell: ShellSession) => new Promise<void>(resolve => shell.once('exit', () => resolve()));
+
+test('only a closed-descriptor error counts as the teardown race', () => {
+  assert.equal(isClosedPtyError(new Error('ioctl(2) failed, EBADF')), true);
+  assert.equal(isClosedPtyError(new Error('ioctl(2) failed, EINVAL')), false);
+  assert.equal(isClosedPtyError('EBADF'), false);
+});
+
+test('resizing a shell that has exited is a no-op, not a crash (threw EBADF before #199)', async () => {
+  const home = scratch('nmsh-resize-');
+  try {
+    const shell = new ShellSession(home, 80, 24, home);
+    await prompt(shell);
+    const exited = exit(shell);
+    shell.write('exit\r');
+    await exited;
+    assert.doesNotThrow(() => shell.resize(90, 30));
+    assert.doesNotThrow(() => shell.resize(1, 1));
+
+    const killed = new ShellSession(home, 80, 24, home);
+    await prompt(killed);
+    const gone = exit(killed);
+    killed.kill();
+    assert.doesNotThrow(() => killed.resize(90, 30), 'resize right after kill');
+    await gone;
+    assert.doesNotThrow(() => killed.resize(91, 31), 'and after the exit is reported');
+  } finally {
+    rmSync(home, {recursive: true, force: true});
+  }
+});
+
+test('a burst of resizes racing the shell\'s exit never throws, and the exit is still reported', async () => {
+  const home = scratch('nmsh-resize-');
+  try {
+    for (let round = 0; round < 5; round += 1) {
+      const shell = new ShellSession(home, 80, 24, home);
+      await prompt(shell);
+      let reported = false;
+      shell.once('exit', () => { reported = true; });
+      shell.write('exit\r');
+      let size = 20;
+      await until(() => {
+        assert.doesNotThrow(() => shell.resize(80 + (size % 7), size % 2 === 0 ? 24 : 25));
+        size += 1;
+        return reported;
+      }, 15000, 'exit reported');
+      for (let index = 0; index < 20; index += 1) shell.resize(100 + index, 30);
+    }
+  } finally {
+    rmSync(home, {recursive: true, force: true});
+  }
+});
+
+test('any other resize failure still surfaces', async () => {
+  const home = scratch('nmsh-resize-');
+  const shell = new ShellSession(home, 80, 24, home);
+  try {
+    await prompt(shell);
+    (shell as unknown as {pty: {resize: () => void}}).pty.resize = () => { throw new Error('ioctl(2) failed, EINVAL'); };
+    assert.throws(() => shell.resize(90, 30), /EINVAL/);
+  } finally {
+    shell.kill();
+    rmSync(home, {recursive: true, force: true});
+  }
+});
+
+class Peer {
+  readonly messages: ServerMessage[] = [];
+  readonly socket: Socket;
+  closed = false;
+  private readonly decoder = new FrameDecoder();
+  constructor(path: string) {
+    this.socket = connect(path);
+    this.socket.setEncoding('utf8');
+    this.socket.on('data', chunk => {
+      for (const result of this.decoder.push(chunk as unknown as string)) if (result.ok) this.messages.push(result.message as ServerMessage);
+    });
+    this.socket.on('close', () => { this.closed = true; });
+    this.socket.on('error', () => {});
+  }
+  send(message: Parameters<typeof encodeMessage>[0]): void { this.socket.write(encodeMessage(message)); }
+  output(): string { return this.messages.map(message => (message.type === 'output' ? message.data : '')).join(''); }
+  async create(home: string): Promise<void> {
+    this.send({type: 'hello', version: PROTOCOL_VERSION, client: 'test'});
+    this.send({type: 'create', cwd: home, env: {HOME: home, PATH: process.env.PATH ?? ''}, columns: 80, rows: 24});
+    await until(() => this.messages.some(message => message.type === 'prompt'), 15000, 'prompt');
+  }
+}
+
+test('nmshd survives resizes that race a shell\'s exit; other sessions keep working and still resize', async () => {
+  const runtimeDir = scratch('nmsh-svc-');
+  const home = scratch('nmsh-home-');
+  const service = new SessionService({runtimeDir, startupIdleMs: 60_000});
+  await service.start();
+  try {
+    const bystander = new Peer(service.socketPath);
+    await bystander.create(home);
+    const racer = new Peer(service.socketPath);
+    await racer.create(home);
+
+    // Resizes before, during and after the exit, including after the service reported it.
+    racer.send({type: 'input', data: 'exit\r'});
+    for (let index = 0; index < 50 && !racer.messages.some(message => message.type === 'exit'); index += 1) {
+      racer.send({type: 'resize', columns: 90 + (index % 5), rows: 30});
+      await new Promise(resolve => setImmediate(resolve));
+    }
+    await until(() => racer.messages.some(message => message.type === 'exit'), 15000, 'racer exit');
+    if (!racer.closed) for (let index = 0; index < 10; index += 1) racer.send({type: 'resize', columns: 100, rows: 40});
+    await new Promise(resolve => setTimeout(resolve, 200));
+
+    assert.equal(bystander.closed, false, 'the service is still running');
+    bystander.send({type: 'resize', columns: 111, rows: 33});
+    bystander.send({type: 'input', data: 'echo SIZE-$(stty size | tr " " x)\r'});
+    await until(() => /SIZE-33x111/u.test(bystander.output()), 15000, 'ordinary resize still reaches the shell');
+  } finally {
+    await closeAndDrain(service);
+    rmSync(runtimeDir, {recursive: true, force: true});
+    rmSync(home, {recursive: true, force: true});
+  }
+});
+
+test('an unexpected resize error is reported to that client instead of ending nmshd', async () => {
+  const runtimeDir = scratch('nmsh-svc-');
+  const home = scratch('nmsh-home-');
+  const service = new SessionService({runtimeDir, startupIdleMs: 60_000});
+  await service.start();
+  try {
+    const peer = new Peer(service.socketPath);
+    await peer.create(home);
+    const managed = [...(service as unknown as {sessions: Map<string, {shell: ShellSession}>}).sessions.values()][0]!;
+    (managed.shell as unknown as {resize: () => void}).resize = () => { throw new Error('ioctl(2) failed, EINVAL'); };
+    peer.send({type: 'resize', columns: 90, rows: 30});
+    await until(() => peer.messages.some(message => message.type === 'error' && message.code === 'resize'), 15000, 'resize error reported');
+    peer.send({type: 'input', data: 'echo STILL-ALIVE\r'});
+    await until(() => /STILL-ALIVE/u.test(peer.output()), 15000, 'session still usable');
+    assert.equal(existsSync(service.socketPath), true);
+  } finally {
+    await closeAndDrain(service);
+    rmSync(runtimeDir, {recursive: true, force: true});
+    rmSync(home, {recursive: true, force: true});
+  }
+});