diff --git a/CHANGELOG.md b/CHANGELOG.md index acfad7c..312ca88 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/session/SessionService.ts b/src/session/SessionService.ts index 9a39cba..47b8060 100644 --- a/src/session/SessionService.ts +++ b/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, 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. diff --git a/src/shell/ShellSession.ts b/src/shell/ShellSession.ts index 6f774cf..ee43888 100644 --- a/src/shell/ShellSession.ts +++ b/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 { 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 { diff --git a/tests/ptyResizeRace.test.ts b/tests/ptyResizeRace.test.ts new file mode 100644 index 0000000..33d9b1b --- /dev/null +++ b/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 { + 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(resolve => shell.once('prompt', () => resolve())); +const exit = (shell: ShellSession) => new Promise(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[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 { + 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}).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}); + } +});